* Re: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag to optimize signaling the host
2015-05-06 18:20 [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag to optimize signaling the host K. Y. Srinivasan
@ 2015-05-06 18:08 ` Eric Dumazet
2015-05-06 18:28 ` KY Srinivasan
0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2015-05-06 18:08 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: davem, netdev, linux-kernel, devel, olaf, apw, jasowang
On Wed, 2015-05-06 at 11:20 -0700, K. Y. Srinivasan wrote:
> Based on the information given to this driver (via the xmit_more skb flag),
> we can defer signaling the host if more packets are on the way. This will help
> make the host more efficient since it can potentially process a larger batch of
> packets. Implement this optimization.
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 5993c7e..4efaa6e 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -435,6 +435,8 @@ check_size:
> packet->page_buf = page_buf;
>
> packet->q_idx = skb_get_queue_mapping(skb);
> + if (netif_tx_queue_stopped(netdev_get_tx_queue(net, packet->q_idx)))
> + packet->xmit_more = false;
Have you tested this condition ever triggers ?
It should not.
netvsc_start_xmit() should not be called if the queue is stopped.
The problem is the following :
netvsc_start_xmit() is called with the packet that is going to stop the
queue, filling last slot, but with skb->xmit_more = 1;
So you need to do something about xmit_more, after calling
netif_tx_stop_queue(), not before.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag to optimize signaling the host
@ 2015-05-06 18:20 K. Y. Srinivasan
2015-05-06 18:08 ` Eric Dumazet
0 siblings, 1 reply; 8+ messages in thread
From: K. Y. Srinivasan @ 2015-05-06 18:20 UTC (permalink / raw)
To: davem, netdev, linux-kernel, devel, olaf, apw, jasowang; +Cc: K. Y. Srinivasan
Based on the information given to this driver (via the xmit_more skb flag),
we can defer signaling the host if more packets are on the way. This will help
make the host more efficient since it can potentially process a larger batch of
packets. Implement this optimization.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
v2: Fixed up indentation based on feedback from David Miller.
v3: If the queue is stopped, deal with that condition: Eric Dumazet <eric.dumazet@gmail.com>
drivers/net/hyperv/netvsc.c | 20 ++++++++++++--------
drivers/net/hyperv/netvsc_drv.c | 2 ++
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index ea091bc..36fef17 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -743,6 +743,7 @@ static inline int netvsc_send_pkt(
u64 req_id;
int ret;
struct hv_page_buffer *pgbuf;
+ u32 vmbus_flags = VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED;
nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT;
if (packet->is_data_pkt) {
@@ -772,19 +773,22 @@ static inline int netvsc_send_pkt(
if (packet->page_buf_cnt) {
pgbuf = packet->cp_partial ? packet->page_buf +
packet->rmsg_pgcnt : packet->page_buf;
- ret = vmbus_sendpacket_pagebuffer(out_channel,
- pgbuf,
- packet->page_buf_cnt,
- &nvmsg,
- sizeof(struct nvsp_message),
- req_id);
+ ret = vmbus_sendpacket_pagebuffer_ctl(out_channel,
+ pgbuf,
+ packet->page_buf_cnt,
+ &nvmsg,
+ sizeof(struct
+ nvsp_message),
+ req_id,
+ vmbus_flags,
+ !packet->xmit_more);
} else {
- ret = vmbus_sendpacket(
+ ret = vmbus_sendpacket_ctl(
out_channel, &nvmsg,
sizeof(struct nvsp_message),
req_id,
VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ vmbus_flags, !packet->xmit_more);
}
if (ret == 0) {
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 5993c7e..4efaa6e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -435,6 +435,8 @@ check_size:
packet->page_buf = page_buf;
packet->q_idx = skb_get_queue_mapping(skb);
+ if (netif_tx_queue_stopped(netdev_get_tx_queue(net, packet->q_idx)))
+ packet->xmit_more = false;
packet->is_data_pkt = true;
packet->total_data_buflen = skb->len;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag to optimize signaling the host
2015-05-06 18:08 ` Eric Dumazet
@ 2015-05-06 18:28 ` KY Srinivasan
0 siblings, 0 replies; 8+ messages in thread
From: KY Srinivasan @ 2015-05-06 18:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, netdev, linux-kernel, devel, olaf, apw, jasowang
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2222 bytes --]
> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Wednesday, May 6, 2015 11:09 AM
> To: KY Srinivasan
> Cc: davem@davemloft.net; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag
> to optimize signaling the host
>
> On Wed, 2015-05-06 at 11:20 -0700, K. Y. Srinivasan wrote:
> > Based on the information given to this driver (via the xmit_more skb flag),
> > we can defer signaling the host if more packets are on the way. This will
> help
> > make the host more efficient since it can potentially process a larger batch
> of
> > packets. Implement this optimization.
>
>
> > diff --git a/drivers/net/hyperv/netvsc_drv.c
> b/drivers/net/hyperv/netvsc_drv.c
> > index 5993c7e..4efaa6e 100644
> > --- a/drivers/net/hyperv/netvsc_drv.c
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -435,6 +435,8 @@ check_size:
> > packet->page_buf = page_buf;
> >
> > packet->q_idx = skb_get_queue_mapping(skb);
> > + if (netif_tx_queue_stopped(netdev_get_tx_queue(net, packet-
> >q_idx)))
> > + packet->xmit_more = false;
>
> Have you tested this condition ever triggers ?
>
> It should not.
>
> netvsc_start_xmit() should not be called if the queue is stopped.
>
> The problem is the following :
>
> netvsc_start_xmit() is called with the packet that is going to stop the
> queue, filling last slot, but with skb->xmit_more = 1;
>
> So you need to do something about xmit_more, after calling
> netif_tx_stop_queue(), not before.
Ah! I too was wondering how we could get into this situation. The condition you mention
is already handled in the lower level - if the attempt to put the last packet on vmbus were to
fail because the ring is full, we will notify the host independent of kick_q parameter - look
at the function vmbus_sendpacket_ctl() in drivers/hv/channel.c
I will resend the patch by reverting this to version 2.
Regards,
K. Y
>
>
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag to optimize signaling the host
@ 2015-05-06 18:28 ` KY Srinivasan
0 siblings, 0 replies; 8+ messages in thread
From: KY Srinivasan @ 2015-05-06 18:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, netdev, linux-kernel, devel, olaf, apw, jasowang
> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Wednesday, May 6, 2015 11:09 AM
> To: KY Srinivasan
> Cc: davem@davemloft.net; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag
> to optimize signaling the host
>
> On Wed, 2015-05-06 at 11:20 -0700, K. Y. Srinivasan wrote:
> > Based on the information given to this driver (via the xmit_more skb flag),
> > we can defer signaling the host if more packets are on the way. This will
> help
> > make the host more efficient since it can potentially process a larger batch
> of
> > packets. Implement this optimization.
>
>
> > diff --git a/drivers/net/hyperv/netvsc_drv.c
> b/drivers/net/hyperv/netvsc_drv.c
> > index 5993c7e..4efaa6e 100644
> > --- a/drivers/net/hyperv/netvsc_drv.c
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -435,6 +435,8 @@ check_size:
> > packet->page_buf = page_buf;
> >
> > packet->q_idx = skb_get_queue_mapping(skb);
> > + if (netif_tx_queue_stopped(netdev_get_tx_queue(net, packet-
> >q_idx)))
> > + packet->xmit_more = false;
>
> Have you tested this condition ever triggers ?
>
> It should not.
>
> netvsc_start_xmit() should not be called if the queue is stopped.
>
> The problem is the following :
>
> netvsc_start_xmit() is called with the packet that is going to stop the
> queue, filling last slot, but with skb->xmit_more = 1;
>
> So you need to do something about xmit_more, after calling
> netif_tx_stop_queue(), not before.
Ah! I too was wondering how we could get into this situation. The condition you mention
is already handled in the lower level - if the attempt to put the last packet on vmbus were to
fail because the ring is full, we will notify the host independent of kick_q parameter - look
at the function vmbus_sendpacket_ctl() in drivers/hv/channel.c
I will resend the patch by reverting this to version 2.
Regards,
K. Y
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag to optimize signaling the host
2015-05-06 18:28 ` KY Srinivasan
@ 2015-05-06 19:17 ` Eric Dumazet
-1 siblings, 0 replies; 8+ messages in thread
From: Eric Dumazet @ 2015-05-06 19:17 UTC (permalink / raw)
To: KY Srinivasan; +Cc: davem, netdev, linux-kernel, devel, olaf, apw, jasowang
On Wed, 2015-05-06 at 18:28 +0000, KY Srinivasan wrote:
> Ah! I too was wondering how we could get into this situation. The condition you mention
> is already handled in the lower level - if the attempt to put the last packet on vmbus were to
> fail because the ring is full, we will notify the host independent of kick_q parameter - look
> at the function vmbus_sendpacket_ctl() in drivers/hv/channel.c
>
> I will resend the patch by reverting this to version 2.
I am not really convinced by what you are saying ;)
problem is not when you fail to add an additional packet.
Problem is :
You queue a packet but not kick because skb->xmit_more = 1, and _then_
you call netif_tx_stop_queue().
No further packet will be delivered to your driver, until
netif_tx_wake_queue() is called again from netvsc_send_completion(),
possibly milli seconds later.
If qdisc was replaced by another one, no packet will kick again, unless
other traffic comes.
Given complexity of your driver, I have no easy way to check you made
this xmit_more conversion correctly, and one packet can not sit in the
equivalent of TX ring buffer for unbound time.
Better add a full explanation into your changelog.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag to optimize signaling the host
@ 2015-05-06 19:17 ` Eric Dumazet
0 siblings, 0 replies; 8+ messages in thread
From: Eric Dumazet @ 2015-05-06 19:17 UTC (permalink / raw)
To: KY Srinivasan; +Cc: olaf, netdev, jasowang, linux-kernel, apw, devel, davem
On Wed, 2015-05-06 at 18:28 +0000, KY Srinivasan wrote:
> Ah! I too was wondering how we could get into this situation. The condition you mention
> is already handled in the lower level - if the attempt to put the last packet on vmbus were to
> fail because the ring is full, we will notify the host independent of kick_q parameter - look
> at the function vmbus_sendpacket_ctl() in drivers/hv/channel.c
>
> I will resend the patch by reverting this to version 2.
I am not really convinced by what you are saying ;)
problem is not when you fail to add an additional packet.
Problem is :
You queue a packet but not kick because skb->xmit_more = 1, and _then_
you call netif_tx_stop_queue().
No further packet will be delivered to your driver, until
netif_tx_wake_queue() is called again from netvsc_send_completion(),
possibly milli seconds later.
If qdisc was replaced by another one, no packet will kick again, unless
other traffic comes.
Given complexity of your driver, I have no easy way to check you made
this xmit_more conversion correctly, and one packet can not sit in the
equivalent of TX ring buffer for unbound time.
Better add a full explanation into your changelog.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag to optimize signaling the host
2015-05-06 19:17 ` Eric Dumazet
@ 2015-05-06 19:55 ` KY Srinivasan
-1 siblings, 0 replies; 8+ messages in thread
From: KY Srinivasan @ 2015-05-06 19:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, netdev, linux-kernel, devel, olaf, apw, jasowang
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2191 bytes --]
> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Wednesday, May 6, 2015 12:18 PM
> To: KY Srinivasan
> Cc: davem@davemloft.net; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag
> to optimize signaling the host
>
> On Wed, 2015-05-06 at 18:28 +0000, KY Srinivasan wrote:
>
> > Ah! I too was wondering how we could get into this situation. The condition
> you mention
> > is already handled in the lower level - if the attempt to put the last packet
> on vmbus were to
> > fail because the ring is full, we will notify the host independent of kick_q
> parameter - look
> > at the function vmbus_sendpacket_ctl() in drivers/hv/channel.c
> >
> > I will resend the patch by reverting this to version 2.
>
> I am not really convinced by what you are saying ;)
>
> problem is not when you fail to add an additional packet.
>
> Problem is :
>
> You queue a packet but not kick because skb->xmit_more = 1, and _then_
> you call netif_tx_stop_queue().
>
> No further packet will be delivered to your driver, until
> netif_tx_wake_queue() is called again from netvsc_send_completion(),
> possibly milli seconds later.
Ok; I see your point. We stop the q in our driver under two conditions:
1. When the ring is full and we fail to queue. This situation is taken care of already.
2. We fall below a certain free space in the ring buffer - this is the case you are pointing out.
I will address this and resend the patch. Thanks for the review.
Regards,
K. Y
>
> If qdisc was replaced by another one, no packet will kick again, unless
> other traffic comes.
>
> Given complexity of your driver, I have no easy way to check you made
> this xmit_more conversion correctly, and one packet can not sit in the
> equivalent of TX ring buffer for unbound time.
>
> Better add a full explanation into your changelog.
>
>
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag to optimize signaling the host
@ 2015-05-06 19:55 ` KY Srinivasan
0 siblings, 0 replies; 8+ messages in thread
From: KY Srinivasan @ 2015-05-06 19:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: olaf, netdev, jasowang, linux-kernel, apw, devel, davem
> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Wednesday, May 6, 2015 12:18 PM
> To: KY Srinivasan
> Cc: davem@davemloft.net; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag
> to optimize signaling the host
>
> On Wed, 2015-05-06 at 18:28 +0000, KY Srinivasan wrote:
>
> > Ah! I too was wondering how we could get into this situation. The condition
> you mention
> > is already handled in the lower level - if the attempt to put the last packet
> on vmbus were to
> > fail because the ring is full, we will notify the host independent of kick_q
> parameter - look
> > at the function vmbus_sendpacket_ctl() in drivers/hv/channel.c
> >
> > I will resend the patch by reverting this to version 2.
>
> I am not really convinced by what you are saying ;)
>
> problem is not when you fail to add an additional packet.
>
> Problem is :
>
> You queue a packet but not kick because skb->xmit_more = 1, and _then_
> you call netif_tx_stop_queue().
>
> No further packet will be delivered to your driver, until
> netif_tx_wake_queue() is called again from netvsc_send_completion(),
> possibly milli seconds later.
Ok; I see your point. We stop the q in our driver under two conditions:
1. When the ring is full and we fail to queue. This situation is taken care of already.
2. We fall below a certain free space in the ring buffer - this is the case you are pointing out.
I will address this and resend the patch. Thanks for the review.
Regards,
K. Y
>
> If qdisc was replaced by another one, no packet will kick again, unless
> other traffic comes.
>
> Given complexity of your driver, I have no easy way to check you made
> this xmit_more conversion correctly, and one packet can not sit in the
> equivalent of TX ring buffer for unbound time.
>
> Better add a full explanation into your changelog.
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-05-06 19:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-06 18:20 [PATCH V3 net-next 1/1] hv_netvsc: Use the xmit_more skb flag to optimize signaling the host K. Y. Srinivasan
2015-05-06 18:08 ` Eric Dumazet
2015-05-06 18:28 ` KY Srinivasan
2015-05-06 18:28 ` KY Srinivasan
2015-05-06 19:17 ` Eric Dumazet
2015-05-06 19:17 ` Eric Dumazet
2015-05-06 19:55 ` KY Srinivasan
2015-05-06 19:55 ` KY Srinivasan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.