> -----Original Message----- > From: Christoph Hellwig [mailto:h...@infradead.org] > Sent: Tuesday, May 10, 2011 3:07 AM > To: KY Srinivasan > Cc: gre...@suse.de; linux-ker...@vger.kernel.org; > de...@linuxdriverproject.org; virtualizat...@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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
