Signed-off-by: Solomon Peachy <[email protected]>
This patch tweaks the skb_copy_bits() call in skb_copy() and
skb_copy_expand(). In the sace of skb_copy():
if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))
Basically, this call assumes that n->head+headerlen == n->data.
This is, fortunately, generally true. But if the alloc_skb function
allocates extra head room (ie calls skb_reserve() on the skb before it
passes it to the callee, this doesn't quite work. Instead, it should be
rewritten as:
if (skb_copy_bits(skb, -headerlen, n->data-headerlen, headerlen + skb->len))
Rewriting it this way works; n->data-headerlen is equal to n->data
before the skb_reserve() call. This seems MoreCorrect(tm), as it makes
no assumptions about the state of the skb passed into it. (n->data just
so happens to equal n->head too)
skb_copy_expand() has the same problem as well, and has a similar fix.
This patch is against 2.6.12-rc4, though it should apply cleanly to any
2.4/2.6 kernel.
...
The history behind this is a little sordid -- We were trying to
implement a "poor man's zerocopy" transmit path for a braindead USB
wireless controller. It needed a descriptor packet prepended to the
frame contents, but couldn't handle it in a separate USB packet -- so
we'd have to do a realloc on the skb to give us the headroom we eneded.
memcpy()s on the very underpowered target were expensive, so we tried
modifying skb_alloc to always ensure there would be enough headroom for
the descriptor (allocating extra, and then skb_reserve()ing it). It was
a crude hack, but it gained us a few much-needed percentage points of
throughput. That is once we fixed skb_copy()..
Anyway, please consider this patch for inclusion.
- Solomon
--
Solomon Peachy ICQ: 1318344
Melbourne, FL JID: [email protected]
Quidquid latine dictum sit, altum viditur
Stuffed Crust <[email protected]> wrote:
>
> This is, fortunately, generally true. But if the alloc_skb function
> allocates extra head room (ie calls skb_reserve() on the skb before it
> passes it to the callee, this doesn't quite work. Instead, it should be
> rewritten as:
As far as I know the alloc_skb funciton in the kernel tree doesn't do
that so your patch is not necessary unless we decide to change the way
alloc_skb works. If that's what you want then please provide a patch
to alloc_skb and a rationale as to why we should do that.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Sun, 8 May 2005 10:32:59 -0400
Stuffed Crust <[email protected]> wrote:
> But if the alloc_skb function
> allocates extra head room (ie calls skb_reserve() on the skb before it
> passes it to the callee, this doesn't quite work.
alloc_skb() may call skb_reserve() in your tree, but it doesn't in anyone
else's.
On Mon, May 09, 2005 at 01:04:34PM +1000, Herbert Xu wrote:
> > This is, fortunately, generally true. But if the alloc_skb function
> > allocates extra head room (ie calls skb_reserve() on the skb before it
> > passes it to the callee, this doesn't quite work. Instead, it should be
> > rewritten as:
>
> As far as I know the alloc_skb funciton in the kernel tree doesn't do
> that so your patch is not necessary unless we decide to change the way
> alloc_skb works. If that's what you want then please provide a patch
> to alloc_skb and a rationale as to why we should do that.
It does not, and I have no intention of submitting a patch to change it.
As I said in my original message, it was a crude hack which has since
been relegated to the great bitbucket of the sky. All that's left is
that "bugfix" patch.
I've performed my due-diligence in airing it to the powers that be, so
I'll go way now.
- Solomon
--
Solomon Peachy ICQ: 1318344
Melbourne, FL JID: [email protected]
Quidquid latine dictum sit, altum viditur