Re: Network performance with small packets

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Feb 01, 2011 at 12:25:08PM -0800, Shirley Ma wrote:
> On Tue, 2011-02-01 at 22:17 +0200, Michael S. Tsirkin wrote:
> > On Tue, Feb 01, 2011 at 12:09:03PM -0800, Shirley Ma wrote:
> > > On Tue, 2011-02-01 at 19:23 +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Jan 27, 2011 at 01:30:38PM -0800, Shirley Ma wrote:
> > > > > On Thu, 2011-01-27 at 13:02 -0800, David Miller wrote:
> > > > > > > Interesting. Could this is be a variant of the now famuous
> > > > > > bufferbloat then?
> > > > > > 
> > > > > > Sigh, bufferbloat is the new global warming... :-/ 
> > > > > 
> > > > > Yep, some places become colder, some other places become warmer;
> > > > Same as
> > > > > BW results, sometimes faster, sometimes slower. :)
> > > > > 
> > > > > Shirley
> > > > 
> > > > Sent a tuning patch (v2) that might help.
> > > > Could you try it and play with the module parameters please? 
> > > 
> > > Hello Michael,
> > > 
> > > Sure I will play with this patch to see how it could help. 
> > > 
> > > I am looking at guest side as well, I found a couple issues on guest
> > > side:
> > > 
> > > 1. free_old_xmit_skbs() should return the number of skbs instead of
> > the
> > > total of sgs since we are using ring size to stop/start netif queue.
> > > static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)
> > > {
> > >         struct sk_buff *skb;
> > >         unsigned int len, tot_sgs = 0;
> > > 
> > >         while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> > >                 pr_debug("Sent skb %p\n", skb);
> > >                 vi->dev->stats.tx_bytes += skb->len;
> > >                 vi->dev->stats.tx_packets++;
> > >                 tot_sgs += skb_vnet_hdr(skb)->num_sg;
> > >                 dev_kfree_skb_any(skb);
> > >         }
> > >         return tot_sgs; <---- should return numbers of skbs to track
> > > ring usage here, I think;
> > > }
> > > 
> > > Did the old guest use number of buffers to track ring usage before?
> > > 
> > > 2. In start_xmit, I think we should move capacity +=
> > free_old_xmit_skbs
> > > before netif_stop_queue(); so we avoid unnecessary netif queue
> > > stop/start. This condition is heavily hit for small message size.
> > > 
> > > Also we capacity checking condition should change to something like
> > half
> > > of the vring.num size, instead of comparing 2+MAX_SKB_FRAGS?
> > > 
> > >        if (capacity < 2+MAX_SKB_FRAGS) {
> > >                 netif_stop_queue(dev);
> > >                 if (unlikely(!virtqueue_enable_cb(vi->svq))) {
> > >                         /* More just got used, free them then
> > recheck.
> > > */
> > >                         capacity += free_old_xmit_skbs(vi);
> > >                         if (capacity >= 2+MAX_SKB_FRAGS) {
> > >                                 netif_start_queue(dev);
> > >                                 virtqueue_disable_cb(vi->svq);
> > >                         }
> > >                 }
> > >         }
> > > 
> > > 3. Looks like the xmit callback is only used to wake the queue when
> > the
> > > queue has stopped, right? Should we put a condition check here?
> > > static void skb_xmit_done(struct virtqueue *svq)
> > > {
> > >         struct virtnet_info *vi = svq->vdev->priv;
> > > 
> > >         /* Suppress further interrupts. */
> > >         virtqueue_disable_cb(svq);
> > > 
> > >         /* We were probably waiting for more output buffers. */
> > > --->   if (netif_queue_stopped(vi->dev))
> > >         netif_wake_queue(vi->dev);
> > > }
> > > 
> > > 
> > > Shirley
> > 
> > Well the return value is used to calculate capacity and that counts
> > the # of s/g. No?
> 
> Nope, the current guest kernel uses descriptors not number of sgs.

Confused. We compare capacity to skb frags, no?
That's sg I think ...

> not sure the old guest.
> 
> > From cache utilization POV it might be better to read from the skb and
> > not peek at virtio header though...
> > Pls Cc the lists on any discussions in the future.
> > 
> > -- 
> > MST
> 
> Sorry I missed reply all. :(
> 
> Shirley