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. I am
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
