RE: [RFC PATCH 1/2] hv_netvsc: Split netvsc_revoke_buf() and netvsc_teardown_gpadl()

[Stephen Hemminger]

There are multiple issues with some of the parameter change paths.
Still working on getting something stable. Both upstream, and net-next do have crash issues under concurrent changes.

I don't want Linux doing different workaround than Windows if at all possible; because it means that it would require much wider testing against many different versions.
Ps: WS2008r2 still needs to be supported.

-----Original Message-----
From: Mohammed Gamal [mailto:mgamal@xxxxxxxxxx] 
Sent: Thursday, February 1, 2018 2:34 PM
To: Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx>
Cc: netdev@xxxxxxxxxxxxxxx; otubo@xxxxxxxxxx; Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; vkuznets@xxxxxxxxxx
Subject: Re: [RFC PATCH 1/2] hv_netvsc: Split netvsc_revoke_buf() and netvsc_teardown_gpadl()

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@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > > > ÂÂ
> > > > > Split each of the functions into two for each of send/recv
> > > > > buffers
> > > > > 
> > > > > Signed-off-by: Mohammed Gamal <mgamal@xxxxxxxxxx>ÂÂ
> > > > 
> > > > 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@xxxxxxxxxx>
> > Fixes: 0cf737808ae7 ("hv_netvsc: netvsc_teardown_gpadl() split")
> > Signed-off-by: Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>
> > ---
> > Â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.