Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Xie He <email@example.com>
To: Willem de Bruijn <firstname.lastname@example.org>
Cc: "David S. Miller" <email@example.com>,
Jakub Kicinski <firstname.lastname@example.org>,
John Ogness <email@example.com>,
Eric Dumazet <firstname.lastname@example.org>,
Or Cohen <email@example.com>,
Arnd Bergmann <firstname.lastname@example.org>,
Network Development <email@example.com>,
Eric Dumazet <firstname.lastname@example.org>,
Brian Norris <email@example.com>,
Cong Wang <firstname.lastname@example.org>
Subject: Re: [PATCH net] net/packet: Fix a comment about hard_header_len and headroom allocation
Date: Tue, 8 Sep 2020 04:27:12 -0700 [thread overview]
Message-ID: <CAJht_ENHmL322bS4BnarXLW+GjOC4ioQ8MMtnsqwOhF_gee5Yw@mail.gmail.com> (raw)
On Tue, Sep 8, 2020 at 1:56 AM Willem de Bruijn
> > > > /*
> > > > Assumptions:
> > > > - - if device has no dev->hard_header routine, it adds and removes ll header
> > > > - inside itself. In this case ll header is invisible outside of device,
> > > > - but higher levels still should reserve dev->hard_header_len.
> > > > - Some devices are enough clever to reallocate skb, when header
> > > > - will not fit to reserved space (tunnel), another ones are silly
> > > > - (PPP).
> > > > + - If the device has no dev->header_ops, there is no LL header visible
> > > > + outside of the device. In this case, its hard_header_len should be 0.
> > >
> > > Such a constraint is more robustly captured with a compile time
> > > BUILD_BUG_ON check. Please do add a comment that summarizes why the
> > > invariant holds.
> > I'm not sure how to do this. I guess both header_ops and
> > hard_header_len are assigned at runtime. (Right?) I guess we are not
> > able to check this at compile-time.
> header_ops should be compile constant, and most devices use
> struct initializers for hard_header_len, but of course you're right.
> Perhaps a WARN_ON_ONCE, then.
OK. Thank you for your suggestion! Actually I was not aware of these
macros. So thank you for introducing them to me! I'll surely add this
> > > More about the older comment, but if reusing: it's not entirely clear
> > > to me what "outside of the device" means. The upper layers that
> > > receive data from the device and send data to it, including
> > > packet_snd, I suppose? Not the lower layers, clearly. Maybe that can
> > > be more specific.
> > Yes, right. If a header is visible "outside of the device", it means
> > the header is exposed to upper layers via "header_ops". If a header is
> > not visible "outside of the device" and is only used "internally", it
> > means the header is not exposed to upper layers via "header_ops".
> > Maybe we can change it to "outside of the device driver"? We can
> > borrow the idea of encapsulation in object-oriented programming - some
> > things that happen inside a software component should not be visible
> > outside of that software component.
> How about "above"? If sketched as a network stack diagram, the code
> paths and devices below the (possibly tunnel) device do see packets
> with link layer header.
OK. I understand what you mean now. We need to make it clear that the
header is only invisible to upper layers but not to "lower layers"
that the device may rely on.
I'm thinking about a way to clearly phrase this. "Above the device"
might be confusing to people. Do you think this is good: "invisible to
upper-layer code including the code in af_packet.c"? Or simply
"invisible to upper-layer code"? Or just "invisible to upper layers"?
(I don't like the last one because I feel according to the network
stack diagram "upper layers" should already and always not care about
the LL header.)
next prev parent reply other threads:[~2020-09-08 11:39 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-06 3:18 Xie He
2020-09-07 9:40 ` Willem de Bruijn
2020-09-08 0:07 ` Xie He
2020-09-08 8:55 ` Willem de Bruijn
2020-09-08 11:27 ` Xie He [this message]
2020-09-08 13:55 ` Willem de Bruijn
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='Re: [PATCH net] net/packet: Fix a comment about hard_header_len and headroom allocation' \
* 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).