All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: Cong Wang <amwang@redhat.com>,
	qemu-devel@nongnu.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org,
	Dave Jones <davej@redhat.com>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH] virtio-net: put virtio net header inline with data
Date: Sun, 9 Jun 2013 10:11:27 +0300	[thread overview]
Message-ID: <20130609071127.GC13082@redhat.com> (raw)
In-Reply-To: <87hahar7ms.fsf@rustcorp.com.au>

On Fri, Jun 07, 2013 at 11:42:43AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > For small packets we can simplify xmit processing by linearizing buffers
> > with the header: most packets seem to have enough head room we can use
> > for this purpose.
> >
> > Since some older hypervisors (e.g. qemu before version 1.5)
> > required that header is the first s/g element,
> > we need a feature bit for this.
> 
> OK, we know this is horrible.  But I will sleep better knowing that we
> this feature need never make it into a final 1.0 spec, since it can be
> assumed at that point...

Nod. Though if we want to require this for all devices,
virtio-blk scsi command passthrough will need to change -
I sent a spec patch a while ago
	virtio-spec: add field for scsi command size
any comments on it?

> >  	pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
> > +	if (vi->mergeable_rx_bufs)
> > +		hdr_len = sizeof hdr->mhdr;
> > +	else
> > +		hdr_len = sizeof hdr->hdr;
> > +
> > +	can_push = vi->any_header_sg &&
> > +		!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
> > +		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
> 
> Idle thought: how often does this fail?

I think it's mostly doesn't fail in my testing.

It's probably a good idea to add a counter here, then
if it starts triggering we can optimize.

I think things like skb_header_cloned depend on guest config
really, e.g. tcpdump running on the interface in guest can cause this.

>  Would it suck if we copied
> headers which didn't let us prepend data?

I think it will - copies are generally best avoided,
and header is easily 1K of data.

>  Or could we bump
> dev->hard_header_len appropriately?

Needs some thought, though from experience it's a pain.


> Thanks,
> Rusty.

WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jason Wang <jasowang@redhat.com>, Cong Wang <amwang@redhat.com>,
	Amos Kong <akong@redhat.com>, Dave Jones <davej@redhat.com>,
	virtualization@lists.linux-foundation.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	qemu-devel@nongnu.org
Subject: Re: [PATCH] virtio-net: put virtio net header inline with data
Date: Sun, 9 Jun 2013 10:11:27 +0300	[thread overview]
Message-ID: <20130609071127.GC13082@redhat.com> (raw)
In-Reply-To: <87hahar7ms.fsf@rustcorp.com.au>

On Fri, Jun 07, 2013 at 11:42:43AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > For small packets we can simplify xmit processing by linearizing buffers
> > with the header: most packets seem to have enough head room we can use
> > for this purpose.
> >
> > Since some older hypervisors (e.g. qemu before version 1.5)
> > required that header is the first s/g element,
> > we need a feature bit for this.
> 
> OK, we know this is horrible.  But I will sleep better knowing that we
> this feature need never make it into a final 1.0 spec, since it can be
> assumed at that point...

Nod. Though if we want to require this for all devices,
virtio-blk scsi command passthrough will need to change -
I sent a spec patch a while ago
	virtio-spec: add field for scsi command size
any comments on it?

> >  	pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
> > +	if (vi->mergeable_rx_bufs)
> > +		hdr_len = sizeof hdr->mhdr;
> > +	else
> > +		hdr_len = sizeof hdr->hdr;
> > +
> > +	can_push = vi->any_header_sg &&
> > +		!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
> > +		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
> 
> Idle thought: how often does this fail?

I think it's mostly doesn't fail in my testing.

It's probably a good idea to add a counter here, then
if it starts triggering we can optimize.

I think things like skb_header_cloned depend on guest config
really, e.g. tcpdump running on the interface in guest can cause this.

>  Would it suck if we copied
> headers which didn't let us prepend data?

I think it will - copies are generally best avoided,
and header is easily 1K of data.

>  Or could we bump
> dev->hard_header_len appropriately?

Needs some thought, though from experience it's a pain.


> Thanks,
> Rusty.

WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: Cong Wang <amwang@redhat.com>,
	qemu-devel@nongnu.org, netdev@vger.kernel.org,
	Jason Wang <jasowang@redhat.com>,
	linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org,
	Dave Jones <davej@redhat.com>, Amos Kong <akong@redhat.com>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [Qemu-devel] [PATCH] virtio-net: put virtio net header inline with data
Date: Sun, 9 Jun 2013 10:11:27 +0300	[thread overview]
Message-ID: <20130609071127.GC13082@redhat.com> (raw)
In-Reply-To: <87hahar7ms.fsf@rustcorp.com.au>

On Fri, Jun 07, 2013 at 11:42:43AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > For small packets we can simplify xmit processing by linearizing buffers
> > with the header: most packets seem to have enough head room we can use
> > for this purpose.
> >
> > Since some older hypervisors (e.g. qemu before version 1.5)
> > required that header is the first s/g element,
> > we need a feature bit for this.
> 
> OK, we know this is horrible.  But I will sleep better knowing that we
> this feature need never make it into a final 1.0 spec, since it can be
> assumed at that point...

Nod. Though if we want to require this for all devices,
virtio-blk scsi command passthrough will need to change -
I sent a spec patch a while ago
	virtio-spec: add field for scsi command size
any comments on it?

> >  	pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
> > +	if (vi->mergeable_rx_bufs)
> > +		hdr_len = sizeof hdr->mhdr;
> > +	else
> > +		hdr_len = sizeof hdr->hdr;
> > +
> > +	can_push = vi->any_header_sg &&
> > +		!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
> > +		!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
> 
> Idle thought: how often does this fail?

I think it's mostly doesn't fail in my testing.

It's probably a good idea to add a counter here, then
if it starts triggering we can optimize.

I think things like skb_header_cloned depend on guest config
really, e.g. tcpdump running on the interface in guest can cause this.

>  Would it suck if we copied
> headers which didn't let us prepend data?

I think it will - copies are generally best avoided,
and header is easily 1K of data.

>  Or could we bump
> dev->hard_header_len appropriately?

Needs some thought, though from experience it's a pain.


> Thanks,
> Rusty.

  reply	other threads:[~2013-06-09  7:11 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-06-06  9:55 [PATCH] virtio-net: put virtio net header inline with data Michael S. Tsirkin
2013-06-06  9:55 ` [Qemu-devel] " Michael S. Tsirkin
2013-06-06 19:59 ` Jesse Larrew
2013-06-06 19:59   ` Jesse Larrew
2013-06-06 20:09   ` Dave Jones
2013-06-06 20:09     ` Dave Jones
2013-06-06 20:09     ` Dave Jones
2013-06-06 20:18     ` Jesse Larrew
2013-06-06 20:18       ` Jesse Larrew
2013-06-07  2:12 ` Rusty Russell
2013-06-07  2:12   ` [Qemu-devel] " Rusty Russell
2013-06-09  7:11   ` Michael S. Tsirkin [this message]
2013-06-09  7:11     ` Michael S. Tsirkin
2013-06-09  7:11     ` Michael S. Tsirkin
2013-06-07  2:52 ` Jason Wang
2013-06-07  2:52   ` [Qemu-devel] " Jason Wang
2013-06-07  2:52   ` Jason Wang
2013-06-09  6:52   ` Michael S. Tsirkin
2013-06-09  6:52     ` [Qemu-devel] " Michael S. Tsirkin
2013-06-09  6:52     ` Michael S. Tsirkin
  -- strict thread matches above, loose matches on Subject: below --
2013-07-08 10:12 Michael S. Tsirkin
2013-07-08 10:12 ` Michael S. Tsirkin
2013-07-09  2:16 ` Rusty Russell
2013-07-09  2:16   ` Rusty Russell
2013-07-09  5:18   ` Michael S. Tsirkin
2013-07-09  5:18     ` Michael S. Tsirkin
2013-07-09  8:08     ` Rusty Russell
2013-07-09  8:08       ` Rusty Russell
2013-07-10  4:38       ` David Miller
2013-07-10  4:38         ` David Miller
2013-07-15  1:43         ` Rusty Russell
2013-07-15  1:43           ` Rusty Russell
2013-07-16 19:33           ` David Miller
2013-07-16 19:33             ` David Miller
2013-07-17  0:08             ` Rusty Russell
2013-07-17  0:08               ` Rusty Russell
2013-07-17  5:00             ` Michael S. Tsirkin
2013-07-17  5:00               ` Michael S. Tsirkin
2013-07-17  5:05               ` David Miller
2013-07-17  5:05                 ` David Miller
2013-07-17  6:02                 ` Rusty Russell
2013-07-17  6:02                   ` Rusty Russell
2013-07-24 19:44                   ` Michael S. Tsirkin
2013-07-24 19:44                     ` Michael S. Tsirkin
2013-07-10  6:14       ` Michael S. Tsirkin
2013-07-10  6:14         ` Michael S. Tsirkin
2013-07-15  1:40         ` Rusty Russell
2013-07-15  1:40           ` Rusty Russell
2013-07-11 13:00   ` Michael S. Tsirkin
2013-07-11 13:00     ` Michael S. Tsirkin
2013-07-12  5:57     ` Rusty Russell
2013-07-12  5:57       ` Rusty Russell

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=20130609071127.GC13082@redhat.com \
    --to=mst@redhat.com \
    --cc=amwang@redhat.com \
    --cc=davej@redhat.com \
    --cc=davem@davemloft.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=qemu-devel@nongnu.org \
    --cc=rusty@rustcorp.com.au \
    --cc=virtualization@lists.linux-foundation.org \
    /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.