[PATCH 4/5] hv: kvp: use wrappers to propaigate state

KY Srinivasan kys at microsoft.com
Mon Sep 21 16:16:36 UTC 2015



> -----Original Message-----
> From: Greg KH [mailto:gregkh at linuxfoundation.org]
> Sent: Sunday, September 20, 2015 10:26 PM
> To: KY Srinivasan <kys at microsoft.com>
> Cc: linux-kernel at vger.kernel.org; devel at linuxdriverproject.org;
> olaf at aepfle.de; apw at canonical.com; vkuznets at redhat.com;
> jasowang at redhat.com
> Subject: Re: [PATCH 4/5] hv: kvp: use wrappers to propaigate state
> 
> On Tue, Sep 15, 2015 at 05:37:53PM -0700, K. Y. Srinivasan wrote:
> > From: Olaf Hering <olaf at aepfle.de>
> >
> > The "state" is used by several threads of execution.
> > Propagate the state to make changes visible. Also propagate context
> > change in kvp_on_msg.
> >
> > Signed-off-by: Olaf Hering <olaf at aepfle.de>
> > Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
> > ---
> >  drivers/hv/hv_kvp.c |   39 +++++++++++++++++++++------------------
> >  1 files changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> > index 74c38a9..778d353 100644
> > --- a/drivers/hv/hv_kvp.c
> > +++ b/drivers/hv/hv_kvp.c
> > @@ -61,7 +61,7 @@
> >   */
> >
> >  static struct {
> > -	int state;   /* hvutil_device_state */
> > +	enum hvutil_device_state state;
> >  	int recv_len; /* number of bytes received. */
> >  	struct hv_kvp_msg  *kvp_msg; /* current message */
> >  	struct vmbus_channel *recv_channel; /* chn we got the request */
> > @@ -74,6 +74,9 @@ static struct {
> >   */
> >  static int dm_reg_value;
> >
> > +#define kvp_get_state()
> hvutil_device_get_state(&kvp_transaction.state)
> > +#define kvp_set_state(s)
> hvutil_device_set_state(&kvp_transaction.state, s)
> > +
> >  static void kvp_send_key(struct work_struct *dummy);
> >
> >
> > @@ -122,8 +125,8 @@ static void kvp_timeout_func(struct work_struct
> *dummy)
> >  	kvp_respond_to_host(NULL, HV_E_FAIL);
> >
> >  	/* Transaction is finished, reset the state. */
> > -	if (kvp_transaction.state > HVUTIL_READY)
> > -		kvp_transaction.state = HVUTIL_READY;
> > +	if (kvp_get_state() > HVUTIL_READY)
> > +		kvp_set_state(HVUTIL_READY);
> >
> 
> And what if the state changed the line after this?  Oops, your code is
> hosed.  See, you need a lock, do this correctly.

This code path is an exception path - request has already been sent to the guest user space and we
are protecting against the guest user space not responding in a reasonable time. Consequently,
the state here has to be >  HVUTIL_READY (we should probably ASSERT this here). When the timeout
triggers, this will be the only code responding back to the host. So there is no issue here and 
I don't think you need a lock here.

The channels for the util driver are all bound to CPU 0. Given this, the simpler solution maybe to 
ensure that we execute all of the various contexts on CPU 0 and have implicit locking.

Regards,

K. Y   











> 
> greg k-h


More information about the devel mailing list