From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH net-next RFC 3/3] virtio-net: conditionally enable tx interrupt Date: Sat, 11 Oct 2014 07:48:19 -0700 Message-ID: <1413038899.9362.43.camel@edumazet-glaptop2.roam.corp.google.com> References: <1413011806-3813-1-git-send-email-jasowang@redhat.com> <1413011806-3813-4-git-send-email-jasowang@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, mst@redhat.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-api@vger.kernel.org To: Jason Wang Return-path: In-Reply-To: <1413011806-3813-4-git-send-email-jasowang@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org List-Id: netdev.vger.kernel.org On Sat, 2014-10-11 at 15:16 +0800, Jason Wang wrote: > We free transmitted packets in ndo_start_xmit() in the past to get better > performance in the past. One side effect is that skb_orphan() needs to be > called in ndo_start_xmit() which makes sk_wmem_alloc not accurate in > fact. For TCP protocol, this means several optimization could not work well > such as TCP small queue and auto corking. This can lead extra low > throughput of small packets stream. > > Thanks to the urgent descriptor support. This patch tries to solve this > issue by enable the tx interrupt selectively for stream packets. This means > we don't need to orphan TCP stream packets in ndo_start_xmit() but enable > tx interrupt for those packets. After we get tx interrupt, a tx napi was > scheduled to free those packets. > > With this method, sk_wmem_alloc of TCP socket were more accurate than in > the past which let TCP can batch more through TSQ and auto corking. > > Signed-off-by: Jason Wang > --- > drivers/net/virtio_net.c | 164 ++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 128 insertions(+), 36 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 5810841..b450fc4 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -72,6 +72,8 @@ struct send_queue { > > /* Name of the send queue: output.$index */ > char name[40]; > + > + struct napi_struct napi; > }; > > /* Internal representation of a receive virtqueue */ > @@ -217,15 +219,40 @@ static struct page *get_a_page(struct receive_queue *rq, gfp_t gfp_mask) > return p; > } > > +static int free_old_xmit_skbs(struct send_queue *sq, int budget) > +{ > + struct sk_buff *skb; > + unsigned int len; > + struct virtnet_info *vi = sq->vq->vdev->priv; > + struct virtnet_stats *stats = this_cpu_ptr(vi->stats); > + int sent = 0; > + > + while (sent < budget && > + (skb = virtqueue_get_buf(sq->vq, &len)) != NULL) { > + pr_debug("Sent skb %p\n", skb); > + > + u64_stats_update_begin(&stats->tx_syncp); > + stats->tx_bytes += skb->len; > + stats->tx_packets++; > + u64_stats_update_end(&stats->tx_syncp); > + > + dev_kfree_skb_any(skb); > + sent++; > + } > + You could accumulate skb->len in a totlen var, and perform a single u64_stats_update_begin(&stats->tx_syncp); stats->tx_bytes += totlen; stats->tx_packets += sent; u64_stats_update_end(&stats->tx_syncp); after the loop. > + return sent; > +} > + ... > + > +static bool virtnet_skb_needs_intr(struct sk_buff *skb) > +{ > + union { > + unsigned char *network; > + struct iphdr *ipv4; > + struct ipv6hdr *ipv6; > + } hdr; > + struct tcphdr *th = tcp_hdr(skb); > + u16 payload_len; > + > + hdr.network = skb_network_header(skb); > + > + /* Only IPv4/IPv6 with TCP is supported */ Oh well, yet another packet flow dissector :) If most packets were caught by your implementation, you could use it for fast patj and fallback to skb_flow_dissect() for encapsulated traffic. struct flow_keys keys; if (!skb_flow_dissect(skb, &keys)) return false; if (keys.ip_proto != IPPROTO_TCP) return false; then check __skb_get_poff() how to get th, and check if there is some payload... > + if ((skb->protocol == htons(ETH_P_IP)) && > + hdr.ipv4->protocol == IPPROTO_TCP) { > + payload_len = ntohs(hdr.ipv4->tot_len) - hdr.ipv4->ihl * 4 - > + th->doff * 4; > + } else if ((skb->protocol == htons(ETH_P_IPV6) || > + hdr.ipv6->nexthdr == IPPROTO_TCP)) { > + payload_len = ntohs(hdr.ipv6->payload_len) - th->doff * 4; > + } else { > + return false; > + } > + > + /* We don't want to dealy packet with PUSH bit and pure ACK packet */ > + if (!th->psh && payload_len) > + return true; > + > + return false; > }