| Message ID | 19254.1447976807@famine |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 19, 2015 at 03:46:47PM -0800, Jay Vosburgh wrote: > > From: K. Y. Srinivasan <kys@microsoft.com> > > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1508706 > > Currently we have two policies for deciding when to signal the host: > One based on the ring buffer state and the other based on what the > VMBUS client driver wants to do. Consider the case when the client > wants to explicitly control when to signal the host. In this case, > if the client were to defer signaling, we will not be able to signal > the host subsequently when the client does want to signal since the > ring buffer state will prevent the signaling. Implement logic to > have only one signaling policy in force for a given channel. > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> > Tested-by: Haiyang Zhang <haiyangz@microsoft.com> > Cc: <stable@vger.kernel.org> # v4.2+ > Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> > --- > drivers/hv/channel.c | 16 ++++++++++++++++ > include/linux/hyperv.h | 12 ++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > index 053dd4cae445..e983d0c92c6c 100644 > --- a/drivers/hv/channel.c > +++ b/drivers/hv/channel.c > @@ -602,6 +602,14 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer, > > ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal); > > + /* Based on the channel signal state, we will decide > + * which signaling policy will be applied. > + */ > + if (channel->signal_state) > + signal = true; > + else > + kick_q = true; > + > if ((ret == 0) && kick_q && signal) > vmbus_setevent(channel); > > @@ -693,6 +701,14 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel, > > ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal); > > + /* Based on the channel signal state, we will decide > + * which signaling policy will be applied. > + */ > + if (channel->signal_state) > + signal = true; > + else > + kick_q = true; > + > if ((ret == 0) && kick_q && signal) > vmbus_setevent(channel); > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index 504b9ec36fe6..0f194ac40326 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -758,8 +758,20 @@ struct vmbus_channel { > * link up channels based on their CPU affinity. > */ > struct list_head percpu_list; > + /* > + * Host signaling policy: The default policy will be > + * based on the ring buffer state. We will also support > + * a policy where the client driver can have explicit > + * signaling control. > + */ > + bool signal_state; > }; > > +static inline void set_channel_signal_state(struct vmbus_channel *c, bool state) > +{ > + c->signal_state = state; > +} > + > static inline void set_channel_read_state(struct vmbus_channel *c, bool state) > { > c->batched_reading = state; > -- > 1.9.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team Positive testing.
On Thu, Nov 19, 2015 at 03:46:47PM -0800, Jay Vosburgh wrote: > > From: K. Y. Srinivasan <kys@microsoft.com> > > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1508706 > > Currently we have two policies for deciding when to signal the host: > One based on the ring buffer state and the other based on what the > VMBUS client driver wants to do. Consider the case when the client > wants to explicitly control when to signal the host. In this case, > if the client were to defer signaling, we will not be able to signal > the host subsequently when the client does want to signal since the > ring buffer state will prevent the signaling. Implement logic to > have only one signaling policy in force for a given channel. > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> > Tested-by: Haiyang Zhang <haiyangz@microsoft.com> > Cc: <stable@vger.kernel.org> # v4.2+ > Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> > --- > drivers/hv/channel.c | 16 ++++++++++++++++ > include/linux/hyperv.h | 12 ++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > index 053dd4cae445..e983d0c92c6c 100644 > --- a/drivers/hv/channel.c > +++ b/drivers/hv/channel.c > @@ -602,6 +602,14 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer, > > ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal); > > + /* Based on the channel signal state, we will decide > + * which signaling policy will be applied. > + */ > + if (channel->signal_state) > + signal = true; > + else > + kick_q = true; > + > if ((ret == 0) && kick_q && signal) > vmbus_setevent(channel); > > @@ -693,6 +701,14 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel, > > ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal); > > + /* Based on the channel signal state, we will decide > + * which signaling policy will be applied. > + */ > + if (channel->signal_state) > + signal = true; > + else > + kick_q = true; > + > if ((ret == 0) && kick_q && signal) > vmbus_setevent(channel); > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index 504b9ec36fe6..0f194ac40326 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -758,8 +758,20 @@ struct vmbus_channel { > * link up channels based on their CPU affinity. > */ > struct list_head percpu_list; > + /* > + * Host signaling policy: The default policy will be > + * based on the ring buffer state. We will also support > + * a policy where the client driver can have explicit > + * signaling control. > + */ > + bool signal_state; > }; > > +static inline void set_channel_signal_state(struct vmbus_channel *c, bool state) > +{ > + c->signal_state = state; > +} > + This introduces a new interface above which is never called. This means that signal_state is presumably 0 always, which means we always set kick_q in the various places. That seems very counter intuitive. > static inline void set_channel_read_state(struct vmbus_channel *c, bool state) > { > c->batched_reading = state; It very much feels like we only have half the story here? Finally, there is already a V2 patch for this, which we likely should be using instead here: https://lkml.org/lkml/2015/11/19/795 Not that that seems to change the fact it has an interface which is never used in it. -apw
On Thu, Nov 19, 2015 at 03:46:47PM -0800, Jay Vosburgh wrote: > > From: K. Y. Srinivasan <kys@microsoft.com> > > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1508706 > > Currently we have two policies for deciding when to signal the host: > One based on the ring buffer state and the other based on what the > VMBUS client driver wants to do. Consider the case when the client > wants to explicitly control when to signal the host. In this case, > if the client were to defer signaling, we will not be able to signal > the host subsequently when the client does want to signal since the > ring buffer state will prevent the signaling. Implement logic to > have only one signaling policy in force for a given channel. > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> > Tested-by: Haiyang Zhang <haiyangz@microsoft.com> > Cc: <stable@vger.kernel.org> # v4.2+ > Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> > --- > drivers/hv/channel.c | 16 ++++++++++++++++ > include/linux/hyperv.h | 12 ++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > index 053dd4cae445..e983d0c92c6c 100644 > --- a/drivers/hv/channel.c > +++ b/drivers/hv/channel.c > @@ -602,6 +602,14 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer, > > ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal); > > + /* Based on the channel signal state, we will decide > + * which signaling policy will be applied. > + */ > + if (channel->signal_state) > + signal = true; > + else > + kick_q = true; > + > if ((ret == 0) && kick_q && signal) > vmbus_setevent(channel); > > @@ -693,6 +701,14 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel, > > ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal); > > + /* Based on the channel signal state, we will decide > + * which signaling policy will be applied. > + */ > + if (channel->signal_state) > + signal = true; > + else > + kick_q = true; > + > if ((ret == 0) && kick_q && signal) > vmbus_setevent(channel); > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index 504b9ec36fe6..0f194ac40326 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -758,8 +758,20 @@ struct vmbus_channel { > * link up channels based on their CPU affinity. > */ > struct list_head percpu_list; > + /* > + * Host signaling policy: The default policy will be > + * based on the ring buffer state. We will also support > + * a policy where the client driver can have explicit > + * signaling control. > + */ > + bool signal_state; > }; > > +static inline void set_channel_signal_state(struct vmbus_channel *c, bool state) > +{ > + c->signal_state = state; > +} > + > static inline void set_channel_read_state(struct vmbus_channel *c, bool state) > { > c->batched_reading = state; I will also note that mainline has the following commit applied too which seems like something we might want to consider in combination with the proposed fix: commit 5f5cc81733baf7b67a039a931dae282e53bf97b9 Author: K. Y. Srinivasan <kys@microsoft.com> Date: Wed Mar 18 12:29:30 2015 -0700 Drivers: hv: vmbus: Fix a siganlling host signalling issue -apw
On Fri, Nov 20, 2015 at 12:16:01PM +0000, Andy Whitcroft wrote: > Finally, there is already a V2 patch for this, which we likely should be > using instead here: > > https://lkml.org/lkml/2015/11/19/795 > > Not that that seems to change the fact it has an interface which is > never used in it. Ok, after some IRC conversation it seems we are willing to revert and replace this with whatever makes it to mainline. As this V1 patch has been functionally tested, and the V2 is only changing a type we may as well stick with the tested one and wait for things to settle upstream. Acked-by: Andy Whitcroft <apw@canonical.com> -apw
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index 053dd4cae445..e983d0c92c6c 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -602,6 +602,14 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer, ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal); + /* Based on the channel signal state, we will decide + * which signaling policy will be applied. + */ + if (channel->signal_state) + signal = true; + else + kick_q = true; + if ((ret == 0) && kick_q && signal) vmbus_setevent(channel); @@ -693,6 +701,14 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel, ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal); + /* Based on the channel signal state, we will decide + * which signaling policy will be applied. + */ + if (channel->signal_state) + signal = true; + else + kick_q = true; + if ((ret == 0) && kick_q && signal) vmbus_setevent(channel); diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 504b9ec36fe6..0f194ac40326 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -758,8 +758,20 @@ struct vmbus_channel { * link up channels based on their CPU affinity. */ struct list_head percpu_list; + /* + * Host signaling policy: The default policy will be + * based on the ring buffer state. We will also support + * a policy where the client driver can have explicit + * signaling control. + */ + bool signal_state; }; +static inline void set_channel_signal_state(struct vmbus_channel *c, bool state) +{ + c->signal_state = state; +} + static inline void set_channel_read_state(struct vmbus_channel *c, bool state) { c->batched_reading = state;