hv_netvsc: Use the xmit_more skb flag to optimize signaling the host
Subsystems (3)
PATCH v3 1/1 hv_netvsc: Use the xmit_more skb flag to optimize signaling the host
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.
---
v2: Fixed up indentation based on feedback from David Miller.
v3: If the queue is stopped, deal with that condition: Eric Dumazet <[email protected]>
drivers/net/hyperv/netvsc.c | 20 ++++++++++++-------- drivers/net/hyperv/netvsc_drv.c | 2 ++ 2 files changed, 14 insertions(+), 8 deletions(-)
drivers/net/hyperv/netvsc.c +12 -8
@@ -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 ) {
drivers/net/hyperv/netvsc_drv.c +2 -0
@@ -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;
Signature · 1 line
1.7.4.1
Quoted 11 lines · K. Y. Srinivasan
Based on the information given to this driver (via the
xmit_moreskb 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.drivers/net/hyperv/netvsc_drv.c +0
-0
drivers/net/hyperv/netvsc_drv.c check_size: · line 439
Quoted 5 lines
drivers/net/hyperv/netvsc_drv.c +2
-0@@ -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.
Quoted 40 lines
-----Original Message-----
From: Eric Dumazet [mailto:[email protected]]
Sent: Wednesday, May 6, 2015 11:09 AM
To: KY Srinivasan
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH V3 net-next 1/1]hv_netvsc: Use thexmit_moreskb flag to optimize signaling the hostQuoted 2 lines · K. Y. Srinivasan
Based on the information given to this driver (via the
xmit_moreskb flag), we can defer signaling the host if more packets are on the way. This willhelp
Quoted 1 line
make the host more efficient since it can potentially process a larger batch
of
Review
Quoted 4 lines
packets. Implement this optimization.
(quoted diff) +0
-0
b/drivers/net/hyperv/netvsc_drv.cdrivers/net/hyperv/netvsc_drv.c check_size: · line 440
Quoted 10 lines
index 5993c7e..4efaa6e 100644
drivers/net/hyperv/netvsc_drv.c +2
-0@@ -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.
Quoted 7 lines
The problem is the following :
netvsc_start_xmit()is called with the packet that is going to stop the queue, filling last slot, but withskb->xmit_more= 1;So you need to do something about
xmit_more, after callingnetif_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
Quoted 2 lines
Quoted 6 lines · KY Srinivasan
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_qparameter - look at the functionvmbus_sendpacket_ctl()indrivers/hv/channel.cI 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.
Quoted 28 lines
-----Original Message-----
From: Eric Dumazet [mailto:[email protected]]
Sent: Wednesday, May 6, 2015 12:18 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH V3 net-next 1/1]hv_netvsc: Use thexmit_moreskb flag to optimize signaling the hostQuoted 1 line · KY Srinivasan
Ah! I too was wondering how we could get into this situation. The condition
you mention
Quoted 1 line
is already handled in the lower level - if the attempt to put the last packet
on vmbus were to
Quoted 1 line
fail because the ring is full, we will notify the host independent of
kick_qparameter - look
Quoted 3 lines
at the function
vmbus_sendpacket_ctl()indrivers/hv/channel.cI 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.
Quoted 8 lines
Problem is :
You queue a packet but not kick because
skb->xmit_more= 1, and _then_ you callnetif_tx_stop_queue().No further packet will be delivered to your driver, until
netif_tx_wake_queue()is called again fromnetvsc_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
Quoted 11 lines
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_moreconversion 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.