All of lore.kernel.org
 help / color / mirror / Atom feed
From: Krishna Kumar2 <krkumar2@in.ibm.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>,
	Carsten Otte <cotte@de.ibm.com>,
	habanero@linux.vnet.ibm.com,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	kvm@vger.kernel.org, lguest@lists.ozlabs.org,
	linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org,
	linux390@de.ibm.com, netdev@vger.kernel.org,
	Rusty Russell <rusty@rustcorp.com.au>,
	Martin Schwidefsky <schwidefsky@de.ibm.com>,
	steved@us.ibm.com, Tom Lendacky <tahm@linux.vnet.ibm.com>,
	virtualization@lists.linux-foundation.org,
	Shirley Ma <xma@us.ibm.com>
Subject: Re: [PATCHv2 10/14] virtio_net: limit xmit polling
Date: Tue, 24 May 2011 18:20:35 +0530	[thread overview]
Message-ID: <OF2D4A7890.FFA91132-ON6525789A.0043E0AC-6525789A.00464690@in.ibm.com> (raw)
In-Reply-To: <20110524112901.GB17087@redhat.com>

"Michael S. Tsirkin" <mst@redhat.com> wrote on 05/24/2011 04:59:39 PM:

> > > > Maybe Rusty means it is a simpler model to free the amount
> > > > of space that this xmit needs. We will still fail anyway
> > > > at some time but it is unlikely, since earlier iteration
> > > > freed up atleast the space that it was going to use.
> > >
> > > Not sure I nderstand.  We can't know space is freed in the previous
> > > iteration as buffers might not have been used by then.
> >
> > Yes, the first few iterations may not have freed up space, but
> > later ones should. The amount of free space should increase
> > from then on, especially since we try to free double of what
> > we consume.
>
> Hmm. This is only an upper limit on the # of entries in the queue.
> Assume that vq size is 4 and we transmit 4 enties without
> getting anything in the used ring. The next transmit will fail.
>
> So I don't really see why it's unlikely that we reach the packet
> drop code with your patch.

I was assuming 256 entries :) I will try to get some
numbers to see how often it is true tomorrow.

> > I am not sure of why it was changed, since returning TX_BUSY
> > seems more efficient IMHO.
> > qdisc_restart() handles requeue'd
> > packets much better than a stopped queue, as a significant
> > part of this code is skipped if gso_skb is present
>
> I think this is the argument:
> http://www.mail-archive.com/virtualization@lists.linux-
> foundation.org/msg06364.html

Thanks for digging up that thread! Yes, that one skb would get
sent first ahead of possibly higher priority skbs. However,
from a performance point, TX_BUSY code skips a lot of checks
and code for all subsequent packets till the device is
restarted. I can test performance with both cases and report
what I find (the requeue code has become very simple and clean
from "horribly complex", thanks to Herbert and Dave).

> > (qdisc
> > will eventually start dropping packets when tx_queue_len is
>
> tx_queue_len is a pretty large buffer so maybe no.

I remember seeing tons of drops (pfifo_fast_enqueue) when
xmit returns TX_BUSY.

> I think the packet drops from the scheduler queue can also be
> done intelligently (e.g. with CHOKe) which should
> work better than dropping a random packet?

I am not sure of that - choke_enqueue checks against a random
skb to drop current skb, and also during congestion. But for
my "sample driver xmit", returning TX_BUSY could still allow
to be used with CHOKe.

thanks,

- KK


WARNING: multiple messages have this Message-ID (diff)
From: Krishna Kumar2 <krkumar2-xthvdsQ13ZrQT0dZR+AlfA@public.gmane.org>
To: "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: habanero-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org,
	lguest-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	Shirley Ma <xma-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>,
	kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Carsten Otte <cotte-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>,
	linux-s390-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Heiko Carstens
	<heiko.carstens-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
	steved-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org,
	Christian Borntraeger
	<borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>,
	Tom Lendacky
	<tahm-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>,
	netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Martin Schwidefsky
	<schwidefsky-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>,
	linux390-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org
Subject: Re: [PATCHv2 10/14] virtio_net: limit xmit polling
Date: Tue, 24 May 2011 18:20:35 +0530	[thread overview]
Message-ID: <OF2D4A7890.FFA91132-ON6525789A.0043E0AC-6525789A.00464690@in.ibm.com> (raw)
In-Reply-To: <20110524112901.GB17087-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

"Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote on 05/24/2011 04:59:39 PM:

> > > > Maybe Rusty means it is a simpler model to free the amount
> > > > of space that this xmit needs. We will still fail anyway
> > > > at some time but it is unlikely, since earlier iteration
> > > > freed up atleast the space that it was going to use.
> > >
> > > Not sure I nderstand.  We can't know space is freed in the previous
> > > iteration as buffers might not have been used by then.
> >
> > Yes, the first few iterations may not have freed up space, but
> > later ones should. The amount of free space should increase
> > from then on, especially since we try to free double of what
> > we consume.
>
> Hmm. This is only an upper limit on the # of entries in the queue.
> Assume that vq size is 4 and we transmit 4 enties without
> getting anything in the used ring. The next transmit will fail.
>
> So I don't really see why it's unlikely that we reach the packet
> drop code with your patch.

I was assuming 256 entries :) I will try to get some
numbers to see how often it is true tomorrow.

> > I am not sure of why it was changed, since returning TX_BUSY
> > seems more efficient IMHO.
> > qdisc_restart() handles requeue'd
> > packets much better than a stopped queue, as a significant
> > part of this code is skipped if gso_skb is present
>
> I think this is the argument:
> http://www.mail-archive.com/virtualization-cunTk1MwBs/ROKNJybVBZg@public.gmane.org
> foundation.org/msg06364.html

Thanks for digging up that thread! Yes, that one skb would get
sent first ahead of possibly higher priority skbs. However,
from a performance point, TX_BUSY code skips a lot of checks
and code for all subsequent packets till the device is
restarted. I can test performance with both cases and report
what I find (the requeue code has become very simple and clean
from "horribly complex", thanks to Herbert and Dave).

> > (qdisc
> > will eventually start dropping packets when tx_queue_len is
>
> tx_queue_len is a pretty large buffer so maybe no.

I remember seeing tons of drops (pfifo_fast_enqueue) when
xmit returns TX_BUSY.

> I think the packet drops from the scheduler queue can also be
> done intelligently (e.g. with CHOKe) which should
> work better than dropping a random packet?

I am not sure of that - choke_enqueue checks against a random
skb to drop current skb, and also during congestion. But for
my "sample driver xmit", returning TX_BUSY could still allow
to be used with CHOKe.

thanks,

- KK

  reply	other threads:[~2011-05-24 12:48 UTC|newest]

Thread overview: 132+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-05-19 23:10 [PATCHv2 00/14] virtio and vhost-net performance enhancements Michael S. Tsirkin
2011-05-19 23:10 ` Michael S. Tsirkin
2011-05-19 23:10 ` [PATCHv2 01/14] virtio: event index interface Michael S. Tsirkin
2011-05-19 23:10   ` Michael S. Tsirkin
2011-05-21  2:29   ` Rusty Russell
2011-05-21  2:29   ` Rusty Russell
2011-05-21  2:29     ` Rusty Russell
2011-05-19 23:10 ` Michael S. Tsirkin
2011-05-19 23:10 ` [PATCHv2 02/14] virtio ring: inline function to check for events Michael S. Tsirkin
2011-05-19 23:10   ` Michael S. Tsirkin
2011-05-21  2:29   ` Rusty Russell
2011-05-21  2:29     ` Rusty Russell
2011-05-21  2:29   ` Rusty Russell
2011-05-19 23:10 ` Michael S. Tsirkin
2011-05-19 23:10 ` [PATCHv2 03/14] virtio_ring: support event idx feature Michael S. Tsirkin
2011-05-19 23:10 ` Michael S. Tsirkin
2011-05-19 23:10   ` Michael S. Tsirkin
2011-05-21  2:31   ` Rusty Russell
2011-05-21  2:31   ` Rusty Russell
2011-05-21  2:31     ` Rusty Russell
2011-05-19 23:10 ` [PATCHv2 04/14] vhost: support event index Michael S. Tsirkin
2011-05-19 23:10   ` Michael S. Tsirkin
2011-05-21  2:31   ` Rusty Russell
2011-05-21  2:31   ` Rusty Russell
2011-05-21  2:31     ` Rusty Russell
2011-05-19 23:10 ` Michael S. Tsirkin
2011-05-19 23:11 ` [PATCHv2 05/14] virtio_test: " Michael S. Tsirkin
2011-05-19 23:11 ` Michael S. Tsirkin
2011-05-19 23:11   ` Michael S. Tsirkin
2011-05-21  2:32   ` Rusty Russell
2011-05-21  2:32   ` Rusty Russell
2011-05-21  2:32     ` Rusty Russell
2011-05-19 23:11 ` [PATCHv2 06/14] virtio: add api for delayed callbacks Michael S. Tsirkin
2011-05-19 23:11 ` Michael S. Tsirkin
2011-05-19 23:11   ` Michael S. Tsirkin
2011-05-21  2:33   ` Rusty Russell
2011-05-21  2:33   ` Rusty Russell
2011-05-21  2:33     ` Rusty Russell
2011-05-19 23:11 ` [PATCHv2 07/14] virtio_net: delay TX callbacks Michael S. Tsirkin
2011-05-19 23:11   ` Michael S. Tsirkin
2011-05-19 23:11 ` Michael S. Tsirkin
2011-05-19 23:11 ` [PATCHv2 08/14] virtio_ring: Add capacity check API Michael S. Tsirkin
2011-05-19 23:11   ` Michael S. Tsirkin
2011-05-19 23:11 ` Michael S. Tsirkin
2011-05-19 23:11 ` [PATCHv2 09/14] virtio_net: fix TX capacity checks using new API Michael S. Tsirkin
2011-05-19 23:11   ` Michael S. Tsirkin
2011-05-21  2:13   ` Rusty Russell
2011-05-21  2:13   ` Rusty Russell
2011-05-21  2:13     ` Rusty Russell
2011-05-19 23:11 ` Michael S. Tsirkin
2011-05-19 23:11 ` [PATCHv2 10/14] virtio_net: limit xmit polling Michael S. Tsirkin
2011-05-19 23:11   ` Michael S. Tsirkin
2011-05-21  2:19   ` Rusty Russell
2011-05-21  2:19   ` Rusty Russell
2011-05-21  2:19     ` Rusty Russell
2011-05-22 12:10     ` Michael S. Tsirkin
2011-05-22 12:10       ` Michael S. Tsirkin
2011-05-23  2:07       ` Rusty Russell
2011-05-23  2:07         ` Rusty Russell
2011-05-23 11:19         ` Michael S. Tsirkin
2011-05-23 11:19         ` Michael S. Tsirkin
2011-05-23 11:19           ` Michael S. Tsirkin
2011-05-24  7:54           ` Krishna Kumar2
2011-05-24  7:54             ` Krishna Kumar2
2011-05-24  9:12             ` Michael S. Tsirkin
2011-05-24  9:12             ` Michael S. Tsirkin
2011-05-24  9:12               ` Michael S. Tsirkin
2011-05-24  9:27               ` Krishna Kumar2
2011-05-24  9:27               ` Krishna Kumar2
2011-05-24 11:29                 ` Michael S. Tsirkin
2011-05-24 11:29                   ` Michael S. Tsirkin
2011-05-24 12:50                   ` Krishna Kumar2 [this message]
2011-05-24 12:50                     ` Krishna Kumar2
2011-05-24 13:52                     ` Michael S. Tsirkin
2011-05-24 13:52                       ` Michael S. Tsirkin
2011-05-24 13:52                     ` Michael S. Tsirkin
2011-05-24 12:50                   ` Krishna Kumar2
2011-05-24 11:29                 ` Michael S. Tsirkin
2011-05-24  7:54           ` Krishna Kumar2
2011-05-25  1:28           ` Rusty Russell
2011-05-25  1:28             ` Rusty Russell
2011-05-25  5:50             ` Michael S. Tsirkin
2011-05-25  5:50             ` Michael S. Tsirkin
2011-05-25  5:50               ` Michael S. Tsirkin
2011-05-25  1:28           ` Rusty Russell
2011-05-25  1:35           ` Rusty Russell
2011-05-25  1:35           ` Rusty Russell
2011-05-25  1:35             ` Rusty Russell
2011-05-25  6:07             ` Michael S. Tsirkin
2011-05-25  6:07               ` Michael S. Tsirkin
2011-05-26  3:28               ` Rusty Russell
2011-05-26  3:28               ` Rusty Russell
2011-05-26  3:28                 ` Rusty Russell
2011-05-28 20:02                 ` Michael S. Tsirkin
2011-05-28 20:02                   ` Michael S. Tsirkin
2011-05-30  6:27                   ` Rusty Russell
2011-05-30  6:27                     ` Rusty Russell
2011-05-30  6:27                   ` Rusty Russell
2011-05-28 20:02                 ` Michael S. Tsirkin
2011-05-25  6:07             ` Michael S. Tsirkin
2011-05-23  2:07       ` Rusty Russell
2011-05-22 12:10     ` Michael S. Tsirkin
2011-05-19 23:11 ` Michael S. Tsirkin
2011-05-19 23:12 ` [PATCHv2 11/14] virtio: don't delay avail index update Michael S. Tsirkin
2011-05-19 23:12   ` Michael S. Tsirkin
2011-05-21  2:26   ` Rusty Russell
2011-05-21  2:26   ` Rusty Russell
2011-05-21  2:26     ` Rusty Russell
2011-05-19 23:12 ` Michael S. Tsirkin
2011-05-19 23:12 ` [PATCHv2 12/14] virtio: 64 bit features Michael S. Tsirkin
2011-05-19 23:12 ` Michael S. Tsirkin
2011-05-19 23:12   ` Michael S. Tsirkin
2011-05-19 23:12 ` [PATCHv2 13/14] virtio_test: update for " Michael S. Tsirkin
2011-05-19 23:12 ` Michael S. Tsirkin
2011-05-19 23:12   ` Michael S. Tsirkin
2011-05-19 23:12 ` [PATCHv2 14/14] vhost: fix " Michael S. Tsirkin
2011-05-19 23:12   ` Michael S. Tsirkin
2011-05-19 23:12 ` Michael S. Tsirkin
2011-05-19 23:20 ` [PATCHv2 00/14] virtio and vhost-net performance enhancements David Miller
2011-05-19 23:20 ` David Miller
2011-05-20  7:51 ` Rusty Russell
2011-05-20  7:51   ` Rusty Russell
2011-05-20  7:51 ` Rusty Russell
2011-05-26 15:32 ` [PERF RESULTS] " Krishna Kumar2
2011-05-26 15:32   ` Krishna Kumar2
2011-05-26 15:42   ` Shirley Ma
2011-05-26 16:21     ` Krishna Kumar2
2011-05-26 16:21     ` Krishna Kumar2
     [not found]     ` <OFF9D0E604.B865A006-ON6525789C.00597010-6525789C.0059987A@LocalDomain>
2011-05-26 16:29       ` Krishna Kumar2
2011-05-26 16:29       ` Krishna Kumar2
2011-05-26 16:29         ` Krishna Kumar2
2011-05-26 15:32 ` Krishna Kumar2

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=OF2D4A7890.FFA91132-ON6525789A.0043E0AC-6525789A.00464690@in.ibm.com \
    --to=krkumar2@in.ibm.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cotte@de.ibm.com \
    --cc=habanero@linux.vnet.ibm.com \
    --cc=heiko.carstens@de.ibm.com \
    --cc=kvm@vger.kernel.org \
    --cc=lguest@lists.ozlabs.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linux390@de.ibm.com \
    --cc=mst@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=rusty@rustcorp.com.au \
    --cc=schwidefsky@de.ibm.com \
    --cc=steved@us.ibm.com \
    --cc=tahm@linux.vnet.ibm.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=xma@us.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.