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

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> Sent: Sunday, September 20, 2015 10:26 PM
> To: KY Srinivasan <kys@xxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx;
> olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx;
> jasowang@xxxxxxxxxx
> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > ---
> >  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
--
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/