[PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in the signalling logic with kick_q
When the caller specifies that signalling should be deferred, we need to address the case where we are not able to place the current packet because the buffer is full. In this case, we will signal the host as some packets may have been placed on the ring buffer. I would like to thank Jason Wang jasow...@redhat.com for pointing out this issue. Signed-off-by: K. Y. Srinivasan k...@microsoft.com --- drivers/hv/channel.c | 32 1 files changed, 32 insertions(+), 0 deletions(-) diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index e58cdb7..ae06ba9 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -614,8 +614,24 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer, ret = hv_ringbuffer_write(channel-outbound, bufferlist, 3, signal); + /* +* Here is the logic for signalling the host: +* 1. If the host is already draining the ringbuffer, +*don't signal. This is indicated by the parameter +*signal. +* +* 2. If we are not able to write, signal if kick_q is false. +*kick_q being false indicates that we may have placed zero or +*more packets with more packets to come. We will signal in +*this case even if potentially we may have not placed any +*packet. This is a rare enough condition that it should not +*matter. +*/ + if ((ret == 0) kick_q signal) vmbus_setevent(channel); + else if ((ret != 0) !kick_q) + vmbus_setevent(channel); return ret; } @@ -705,8 +721,24 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel, ret = hv_ringbuffer_write(channel-outbound, bufferlist, 3, signal); + /* +* Here is the logic for signalling the host: +* 1. If the host is already draining the ringbuffer, +*don't signal. This is indicated by the parameter +*signal. +* +* 2. If we are not able to write, signal if kick_q is false. +*kick_q being false indicates that we may have placed zero or +*more packets with more packets to come. We will signal in +*this case even if potentially we may have not placed any +*packet. This is a rare enough condition that it should not +*matter. +*/ + if ((ret == 0) kick_q signal) vmbus_setevent(channel); + else if ((ret != 0) !kick_q) + vmbus_setevent(channel); return ret; } -- 1.7.4.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in the signalling logic with kick_q
On Thu, Mar 12, 2015 at 3:04 AM, K. Y. Srinivasan k...@microsoft.com wrote: When the caller specifies that signalling should be deferred, we need to address the case where we are not able to place the current packet because the buffer is full. In this case, we will signal the host as some packets may have been placed on the ring buffer. I would like to thank Jason Wang jasow...@redhat.com for pointing out this issue. Signed-off-by: K. Y. Srinivasan k...@microsoft.com --- drivers/hv/channel.c | 32 1 files changed, 32 insertions(+), 0 deletions(-) diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index e58cdb7..ae06ba9 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -614,8 +614,24 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer, ret = hv_ringbuffer_write(channel-outbound, bufferlist, 3, signal); + /* +* Here is the logic for signalling the host: +* 1. If the host is already draining the ringbuffer, +*don't signal. This is indicated by the parameter +*signal. +* +* 2. If we are not able to write, signal if kick_q is false. +*kick_q being false indicates that we may have placed zero or +*more packets with more packets to come. We will signal in +*this case even if potentially we may have not placed any +*packet. This is a rare enough condition that it should not +*matter. +*/ + if ((ret == 0) kick_q signal) vmbus_setevent(channel); + else if ((ret != 0) !kick_q) + vmbus_setevent(channel); return ret; } @@ -705,8 +721,24 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel, ret = hv_ringbuffer_write(channel-outbound, bufferlist, 3, signal); + /* +* Here is the logic for signalling the host: +* 1. If the host is already draining the ringbuffer, +*don't signal. This is indicated by the parameter +*signal. +* +* 2. If we are not able to write, signal if kick_q is false. +*kick_q being false indicates that we may have placed zero or +*more packets with more packets to come. We will signal in +*this case even if potentially we may have not placed any +*packet. This is a rare enough condition that it should not +*matter. +*/ + if ((ret == 0) kick_q signal) vmbus_setevent(channel); + else if ((ret != 0) !kick_q) + vmbus_setevent(channel); return ret; } -- Looks like we need to kick unconditionally here. Consider we may get -EAGAIN when we want to send the last skb (kick_q is true) from the list. We need kick host in this case. Btw, another method is let the driver to decide e.g exporting the vmbus_setevent() and call it in netvsc_start_xmit(). ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in the signalling logic with kick_q
-Original Message- From: Jason Wang [mailto:jasow...@redhat.com] Sent: Wednesday, March 11, 2015 8:08 PM To: KY Srinivasan Cc: da...@davemloft.net; net...@vger.kernel.org; linux- ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; gre...@linuxfoundation.org; KY Srinivasan Subject: Re: [PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in the signalling logic with kick_q On Thu, Mar 12, 2015 at 3:04 AM, K. Y. Srinivasan k...@microsoft.com wrote: When the caller specifies that signalling should be deferred, we need to address the case where we are not able to place the current packet because the buffer is full. In this case, we will signal the host as some packets may have been placed on the ring buffer. I would like to thank Jason Wang jasow...@redhat.com for pointing out this issue. Signed-off-by: K. Y. Srinivasan k...@microsoft.com --- drivers/hv/channel.c | 32 1 files changed, 32 insertions(+), 0 deletions(-) diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index e58cdb7..ae06ba9 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -614,8 +614,24 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer, ret = hv_ringbuffer_write(channel-outbound, bufferlist, 3, signal); + /* +* Here is the logic for signalling the host: +* 1. If the host is already draining the ringbuffer, +*don't signal. This is indicated by the parameter +*signal. +* +* 2. If we are not able to write, signal if kick_q is false. +*kick_q being false indicates that we may have placed zero or +*more packets with more packets to come. We will signal in +*this case even if potentially we may have not placed any +*packet. This is a rare enough condition that it should not +*matter. +*/ + if ((ret == 0) kick_q signal) vmbus_setevent(channel); + else if ((ret != 0) !kick_q) + vmbus_setevent(channel); return ret; } @@ -705,8 +721,24 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel, ret = hv_ringbuffer_write(channel-outbound, bufferlist, 3, signal); + /* +* Here is the logic for signalling the host: +* 1. If the host is already draining the ringbuffer, +*don't signal. This is indicated by the parameter +*signal. +* +* 2. If we are not able to write, signal if kick_q is false. +*kick_q being false indicates that we may have placed zero or +*more packets with more packets to come. We will signal in +*this case even if potentially we may have not placed any +*packet. This is a rare enough condition that it should not +*matter. +*/ + if ((ret == 0) kick_q signal) vmbus_setevent(channel); + else if ((ret != 0) !kick_q) + vmbus_setevent(channel); return ret; } -- Looks like we need to kick unconditionally here. Consider we may get - EAGAIN when we want to send the last skb (kick_q is true) from the list. We need kick host in this case. Btw, another method is let the driver to decide e.g exporting the vmbus_setevent() and call it in netvsc_start_xmit(). There is other state that governs if the host needs to be signaled and I don't think we want to expose that to netvsc. In any case, netvsc will have to signal the host when EGAIN is received. We might as well do it in the vmbus driver. Thank you Jason, I will respin and resubmit the series. K. Y ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in the signalling logic with kick_q
-Original Message- From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf Of KY Srinivasan Sent: Wednesday, March 11, 2015 8:32 PM To: Jason Wang Cc: o...@aepfle.de; net...@vger.kernel.org; linux-ker...@vger.kernel.org; gre...@linuxfoundation.org; a...@canonical.com; de...@linuxdriverproject.org; da...@davemloft.net Subject: RE: [PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in the signalling logic with kick_q -Original Message- From: Jason Wang [mailto:jasow...@redhat.com] Sent: Wednesday, March 11, 2015 8:08 PM To: KY Srinivasan Cc: da...@davemloft.net; net...@vger.kernel.org; linux- ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; gre...@linuxfoundation.org; KY Srinivasan Subject: Re: [PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in the signalling logic with kick_q On Thu, Mar 12, 2015 at 3:04 AM, K. Y. Srinivasan k...@microsoft.com wrote: When the caller specifies that signalling should be deferred, we need to address the case where we are not able to place the current packet because the buffer is full. In this case, we will signal the host as some packets may have been placed on the ring buffer. I would like to thank Jason Wang jasow...@redhat.com for pointing out this issue. Signed-off-by: K. Y. Srinivasan k...@microsoft.com --- drivers/hv/channel.c | 32 1 files changed, 32 insertions(+), 0 deletions(-) diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index e58cdb7..ae06ba9 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -614,8 +614,24 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer, ret = hv_ringbuffer_write(channel-outbound, bufferlist, 3, signal); + /* + * Here is the logic for signalling the host: + * 1. If the host is already draining the ringbuffer, + *don't signal. This is indicated by the parameter + *signal. + * + * 2. If we are not able to write, signal if kick_q is false. + *kick_q being false indicates that we may have placed zero or + *more packets with more packets to come. We will signal in + *this case even if potentially we may have not placed any + *packet. This is a rare enough condition that it should not + *matter. + */ + if ((ret == 0) kick_q signal) vmbus_setevent(channel); + else if ((ret != 0) !kick_q) + vmbus_setevent(channel); return ret; } @@ -705,8 +721,24 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel, ret = hv_ringbuffer_write(channel-outbound, bufferlist, 3, signal); + /* + * Here is the logic for signalling the host: + * 1. If the host is already draining the ringbuffer, + *don't signal. This is indicated by the parameter + *signal. + * + * 2. If we are not able to write, signal if kick_q is false. + *kick_q being false indicates that we may have placed zero or + *more packets with more packets to come. We will signal in + *this case even if potentially we may have not placed any + *packet. This is a rare enough condition that it should not + *matter. + */ + if ((ret == 0) kick_q signal) vmbus_setevent(channel); + else if ((ret != 0) !kick_q) + vmbus_setevent(channel); return ret; } -- Looks like we need to kick unconditionally here. Consider we may get - EAGAIN when we want to send the last skb (kick_q is true) from the list. We need kick host in this case. Btw, another method is let the driver to decide e.g exporting the vmbus_setevent() and call it in netvsc_start_xmit(). There is other state that governs if the host needs to be signaled and I don't think we want to expose that to netvsc. In any case, netvsc will have to signal the host when EGAIN is received. We might as well do it in the vmbus driver. Thank you Jason, I will respin and resubmit the series. Looks like there may be a way to make the signaling more precise even in this case. I am going to experiment with it. K. Y K. Y ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
