From: Eric Dumazet <eric.dumazet@gmail.com>
To: Dave Wiltshire <david.wiltshire@gmx.com>
Cc: davem@davemloft.net, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, viro@zeniv.linux.org.uk,
eparis@redhat.com, edumazet@google.com
Subject: Re: [PATCH 1/3] skbuff: Update truesize in pskb_expand_head
Date: Wed, 12 Jun 2013 22:38:56 -0700 [thread overview]
Message-ID: <1371101936.3252.92.camel@edumazet-glaptop> (raw)
In-Reply-To: <20130612233503.GB10989@linux-rbgc.site>
On Thu, 2013-06-13 at 09:35 +1000, Dave Wiltshire wrote:
> Firstly, from my cover letter: "Perhaps I don't understand something,
> but I thought it best to generate the change and then ask. So is this
> correct?".
Sure I have no problems with that.
> But secondly, I understand that the only reason for truesize
> is for memory accounting on sockets. Indeed that's why I thought this
> was incorrect. Something being complex is not a good reason not to do
> it.
OK but right now your patch adds many regressions, that will take weeks
for other dev to discover, understand and fix.
If you change skb->truesize not properly while skb is owned by a socket,
we can hold references forever and prevent sockets from being
dismantled/freed, or worse we'll free sockets while they are still in
use and panic the machine.
Some callers of pskb_expand_head() do not want skb->truesize to be
modified, because skb might be orphaned or not.
There are hundred of call sites.
Really, your patch is way too risky and will consume too much time for
very little gain. We cannot change conventions without a full audit.
There are some cases where truesize can not be exactly tracked.
For example, when we pull all data from a frag into skb->head, and the
frag can be release (put_page() on it), we do not know its original size
and can not reduce skb->truesize by this amount.
Thats fine, most of the time, because we pull network headers and they
are limited in size.
Your changelog used : "This is likely a memory audit leak", which is
weak considering the amount of time needed for us to validate such a big
change.
prev parent reply other threads:[~2013-06-13 5:38 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-12 9:05 [PATCH 0/3] skbuff: pskb_expand_head changes Dave Wiltshire
2013-06-12 9:05 ` [PATCH 1/3] skbuff: Update truesize in pskb_expand_head Dave Wiltshire
2013-06-12 9:16 ` Eric Dumazet
2013-06-12 23:35 ` Dave Wiltshire
2013-06-13 5:38 ` Eric Dumazet [this message]
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=1371101936.3252.92.camel@edumazet-glaptop \
--to=eric.dumazet@gmail.com \
--cc=davem@davemloft.net \
--cc=david.wiltshire@gmx.com \
--cc=edumazet@google.com \
--cc=eparis@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
/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