On Thu, 2018-02-01 at 09:37 +0100, Mohammed Gamal wrote:
> On Wed, 2018-01-31 at 15:01 -0800, Stephen Hemminger wrote:
> > On Wed, 31 Jan 2018 12:16:49 +0100
> > Mohammed Gamal <mga...@redhat.com> wrote:
> >
> > > On Tue, 2018-01-30 at 11:29 -0800, Stephen Hemminger wrote:
> > > > On Tue, 23 Jan 2018 10:34:04 +0100
> > > > Mohammed Gamal <mga...@redhat.com> wrote:
> > > >
> > > > > Split each of the functions into two for each of send/recv
> > > > > buffers
> > > > >
> > > > > Signed-off-by: Mohammed Gamal <mga...@redhat.com>
> > > >
> > > > Splitting these functions is not necessary
> > >
> > > How so? We need to send each message independently, and hence the
> > > split
> > > (see cover letter). Is there another way?
> >
> > This is all that is needed.
> >
> >
> > Subject: [PATCH] hv_netvsc: work around for gpadl teardown on older
> > windows
> > server
> >
> > On WS2012 the host ignores messages after vmbus channel is closed.
> > Workaround this by doing what Windows does and send the teardown
> > before close on older versions of NVSP protocol.
> >
> > Reported-by: Mohammed Gamal <mga...@redhat.com>
> > Fixes: 0cf737808ae7 ("hv_netvsc: netvsc_teardown_gpadl() split")
> > Signed-off-by: Stephen Hemminger <sthem...@microsoft.com>
> > ---
> > drivers/net/hyperv/netvsc.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/hyperv/netvsc.c
> > b/drivers/net/hyperv/netvsc.c
> > index 17e529af79dc..1a3df0eff42f 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -574,10 +574,17 @@ void netvsc_device_remove(struct hv_device
> > *device)
> > */
> > netdev_dbg(ndev, "net device safe to remove\n");
> >
> > + /* Workaround for older versions of Windows require that
> > + * buffer be revoked before channel is disabled
> > + */
> > + if (net_device->nvsp_version < NVSP_PROTOCOL_VERSION_4)
> > + netvsc_teardown_gpadl(device, net_device);
> > +
> > /* Now, we can close the channel safely */
> > vmbus_close(device->channel);
> >
> > - netvsc_teardown_gpadl(device, net_device);
> > + if (net_device->nvsp_version >= NVSP_PROTOCOL_VERSION_4)
> > + netvsc_teardown_gpadl(device, net_device);
> >
> > /* And dissassociate NAPI context from device */
> > for (i = 0; i < net_device->num_chn; i++)
>
> I've tried a similar workaround before by calling
> netvsc_teardown_gpadl() after netvsc_revoke_buf(), but before setting
> net_device_ctx->nvdev to NULL and it caused the guest to hang when
> trying to change MTU.
>
> Let me try that change and see if it behaves differently.
I tested the patch, but I've actually seen some unexpected behavior.
First, net_device->nvsp_version is actually NVSP_PROTOCOL_VERSION_5 on
both my Win2012 and Win2016 hosts that I tested on, so the condition is
never executed.
Second, when doing the check instead as if (vmbus_proto_version <
VERSION_WIN10), I get the same behavior I described above where the
guest hangs as the kernel waits indefinitely in vmbus_teardown_gpadl()
for a completion to be signaled. This is actually what lead me to
propose splitting netvsc_revoke_buf() and netvsc_teardown_gpadl() in my
initial patchset so that we keep the same order of messages and avoid
that indefinite wait.
