Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Xie He <xie.he.0141@gmail.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Network Development <netdev@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Linux X25 <linux-x25@vger.kernel.org>,
	Martin Schiller <ms@dev.tdt.de>
Subject: Re: [PATCH net] drivers/net/wan/lapbether: Added needed_tailroom
Date: Mon, 10 Aug 2020 11:13:46 -0700	[thread overview]
Message-ID: <CAJht_EPGD1RmnU6-ZJYocXCY-qcPxXeEuurQ6GJod=WGO69-jg@mail.gmail.com> (raw)
In-Reply-To: <CA+FuTSdBNn218kuswND5OE4vZ4mxz3_hTDkcRmZn2Z9-gaYQZg@mail.gmail.com>

On Mon, Aug 10, 2020 at 12:32 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> What happens when a tunnel device passes a packet to these devices?
> That will also not have allocated the extra tailroom. Does that cause
> a bug?

I looked at the code in net/ipv4/ip_tunnel.c. It indeed appeared to me
that it didn't take needed_tailroom into consideration. However it
does take needed_headroom into consideration through the macro
LL_RESERVED_SPACE. I think it would be better for it to take
needed_tailroom into consideration, too.

However, looking at the comment of needed_tailroom in
include/linux/netdevice.h, it says "Extra tailroom the hardware may
need, but not in all cases can this be guaranteed". So if we take this
comment as the spec, we can consider this to be not a bug. The reason
the author of this comment said so, might be that he wanted to add
needed_tailroom to solve some problems, but he was not able to change
all code to take needed_tailroom into consideration, so he wrote in
the comment saying that it was not necessary to always guarantee
needed_tailroom.

If we take this comment as the spec, to prevent bugs, any driver that
sets needed_tailroom must always check (and re-allocate if necessary)
before using the tailroom.

However, I still think it would be better to always take into
consideration needed_tailroom (and needed_headroom, too), so that
eventually we can remove the words of "but not in all cases can this
be guaranteed" from the comment. That would make the code more logical
and consistent.

Thank you for raising this important question about needed_tailroom!

  reply	other threads:[~2020-08-10 18:14 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-08 17:52 Xie He
2020-08-09  8:48 ` Willem de Bruijn
2020-08-09 17:11   ` Xie He
2020-08-10  7:31     ` Willem de Bruijn
2020-08-10 18:13       ` Xie He [this message]
2020-08-16  2:28         ` Xie He
2020-08-19  0:17 ` Xie He
2020-08-19 22:11 ` Xie He

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='CAJht_EPGD1RmnU6-ZJYocXCY-qcPxXeEuurQ6GJod=WGO69-jg@mail.gmail.com' \
    --to=xie.he.0141@gmail.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-x25@vger.kernel.org \
    --cc=ms@dev.tdt.de \
    --cc=netdev@vger.kernel.org \
    --cc=willemdebruijn.kernel@gmail.com \
    --subject='Re: [PATCH net] drivers/net/wan/lapbether: Added needed_tailroom' \
    /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

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).