lkml.org 
[lkml]   [2018]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 1/2] hv_netvsc: Split netvsc_revoke_buf() and netvsc_teardown_gpadl()
From
Date
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 <mgamal@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 <mgamal@redhat.com> wrote:
> > > >   
> > > > > Split each of the functions into two for each of send/recv
> > > > > buffers
> > > > >
> > > > > Signed-off-by: Mohammed Gamal <mgamal@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 <mgamal@redhat.com>
> > Fixes: 0cf737808ae7 ("hv_netvsc: netvsc_teardown_gpadl() split")
> > Signed-off-by: Stephen Hemminger <sthemmin@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.

\
 
 \ /
  Last update: 2018-02-01 23:35    [W:0.474 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site