RE: [PATCH 115/206] Staging: hv: Use completion abstraction instruct netvsc_device

[KY Srinivasan]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@xxxxxxxxxxxxx]
> Sent: Tuesday, May 10, 2011 3:07 AM
> To: KY Srinivasan
> Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/