[PATCH 115/206] Staging: hv: Use completion abstraction in struct netvsc_device

KY Srinivasan kys at microsoft.com
Tue May 10 12:52:07 UTC 2011



> -----Original Message-----
> From: Christoph Hellwig [mailto:hch at infradead.org]
> Sent: Tuesday, May 10, 2011 3:07 AM
> To: KY Srinivasan
> Cc: gregkh at suse.de; linux-kernel at vger.kernel.org;
> devel at linuxdriverproject.org; virtualization at lists.osdl.org; Haiyang Zhang;
> Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen
> Subject: Re: [PATCH 115/206] Staging: hv: Use completion abstraction in struct
> netvsc_device
> 
> > -	net_device->wait_condition = 0;
> >  	ret = vmbus_sendpacket(device->channel, init_packet,
> >  			       sizeof(struct nvsp_message),
> >  			       (unsigned long)init_packet,
> > @@ -272,10 +272,8 @@ static int netvsc_init_recv_buf(struct hv_device
> *device)
> >  		goto cleanup;
> >  	}
> >
> > -	wait_event_timeout(net_device->channel_init_wait,
> > -			net_device->wait_condition,
> > -			msecs_to_jiffies(1000));
> > -	BUG_ON(net_device->wait_condition == 0);
> > +	t = wait_for_completion_timeout(&net_device->channel_init_wait, HZ);
> > +	BUG_ON(t == 0);
> 
> I don't think you want a BUG_ON here, but rather fail the
> initialization.

This is existing code and all I did was change the synchronization 
primitive used. Back in Feb. when this was done (prior to then), the 
wait was indefinite and one of the comments that was
longstanding was  that the wait had to be bounded and so these 
timed waits were introduced. When this was done,  
in some cases, there was no easy way to roll-back state if we did not
get the response within our expected window of time. This is one 
such instance where the guest is handing over the receive buffers
(guest physical addresses) to the host. There was a discussion on this very
topic on this mailing list and the consensus was to leave the BUG_ON()
call in cases where we could not reasonably recover. 
 
> 
> Also I think you really should add synchronous versions of
> vmbus_sendpacket*, to the vmbus core so that all the synchronous waiting
> can be consolidated into a few helpers instead of needing to opencode
> it everywhere.

True; but having a non-blocking interface at the lowest level gives you
the most flexibility. In addition to this non-blocking interface, we
may want to look at potentially a blocking interface. In the current
code, the blocking primitive that is embedded in a higher level 
abstraction is used for a variety of situations including the initial
handshake which is what can be addressed with a synchronous API
at the lowest level.

Regards,

K. Y




More information about the devel mailing list