netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Vrabel <david.vrabel@citrix.com>
To: Paul Durrant <paul.durrant@citrix.com>
Cc: <xen-devel@lists.xen.org>, <netdev@vger.kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Ian Campbell <Ian.Campbell@citrix.com>,
	Annie Li <annie.li@oracle.com>, Matt Wilson <msw@amazon.com>,
	Xi Xiong <xixiong@amazon.com>
Subject: Re: [Xen-devel] [PATCH net-next] xen-netback: fix	xenvif_count_skb_slots()
Date: Mon, 7 Oct 2013 10:50:12 +0100	[thread overview]
Message-ID: <525283D4.7010504@citrix.com> (raw)
In-Reply-To: <1380903983-27429-1-git-send-email-paul.durrant@citrix.com>

On 04/10/13 17:26, Paul Durrant wrote:
> Commit 4f0581d25827d5e864bcf07b05d73d0d12a20a5c introduced an error into
> xenvif_count_skb_slots() for skbs with a linear area spanning a page
> boundary. The alignment of skb->data needs to be taken into account, not
> just the head length. This patch fixes the issue by dry-running the code
> from xenvif_gop_skb() (and adjusting the comment above the function to note
> that).

If 4f0581d2582 is causing the skb->data to be fully packed into a
minimal number of slots then the simple DIV_ROUND_UP(skb_headlen(skb))
is correct.

I think this change will miscount in the number of slots,
over-estimating the count which I think will eventually cause netback to
think the ring has no space when it has some.

Is the problem here not the miscounting of slots but running out of
space in the grant table op array because we know use more copy ops?

I didn't think there was any real merit in the problematic commit (or at
least there was no evidence that it was better) so I would suggest just
reverting it instead of trying to fix it up.

If we do want to change how netback fills the ring then netback needs
some redesign (i.e., change it so it doesn't have to this counting in
advance) to make it much less fragile to changes in this area.

David

  parent reply	other threads:[~2013-10-07  9:50 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-04 16:26 [PATCH net-next] xen-netback: fix xenvif_count_skb_slots() Paul Durrant
2013-10-07  0:06 ` Matt Wilson
2013-10-07  0:07   ` Matt Wilson
2013-10-07 10:17     ` Paul Durrant
2013-10-07  9:50 ` David Vrabel [this message]
2013-10-07 10:23   ` [Xen-devel] " Paul Durrant
2013-10-07 10:37     ` Paul Durrant
2013-10-07 10:53       ` Wei Liu
2013-10-07 10:01 ` Wei Liu
2013-10-07 10:12   ` Paul Durrant
2013-10-07 19:36 ` David Miller
2013-10-07 20:03   ` Ian Campbell

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=525283D4.7010504@citrix.com \
    --to=david.vrabel@citrix.com \
    --cc=Ian.Campbell@citrix.com \
    --cc=annie.li@oracle.com \
    --cc=msw@amazon.com \
    --cc=netdev@vger.kernel.org \
    --cc=paul.durrant@citrix.com \
    --cc=wei.liu2@citrix.com \
    --cc=xen-devel@lists.xen.org \
    --cc=xixiong@amazon.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).