From: Daniel Borkmann <daniel@iogearbox•net>
To: Jakub Kicinski <kubakici@wp•pl>
Cc: davem@davemloft•net, alexei.starovoitov@gmail•com,
john.fastabend@gmail•com, netdev@vger•kernel.org
Subject: Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once
Date: Thu, 11 May 2017 01:02:08 +0200 [thread overview]
Message-ID: <59139BF0.5090606@iogearbox.net> (raw)
In-Reply-To: <20170510154630.316010f9@cakuba.netronome.com>
On 05/11/2017 12:46 AM, Jakub Kicinski wrote:
> On Thu, 11 May 2017 00:24:56 +0200, Daniel Borkmann wrote:
>>> I understand the counter argument that from user space perspective it
>>> would make things slightly more complicated because there would be two
>>> conditions in which driver hook is used:
>>> 1) DRV_MODE set on dump;
>>> 2) flags attribute not present (old kernel).
>>>
>>> I'm concerned about number 2). We can't simply depend on SKB_MODE
>>> not being set because we may add more *_MODE flags in the future. So
>>> doing:
>>>
>>> if (flags & SKB_MODE)
>>> printf("skb-mode");
>>> else
>>> printf("drv-mode");
>>>
>>> is not correct. The flags attribute must not be present at all (think
>>> HW_MODE flag). But going further there can also be non-MODE flags,
>>> like, say.. NEVER_TX, and then there may be flags present in dump,
>>> and if SKB_MODE isn't be set, the mode could be some new MODE user space
>>> doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no
>>> way to tell :S
>>
>> Yep, I see your point. Additionally, if we use XDP_FLAGS_* again for
>> dumping we're wasting bit space for flags we would never dump back
>> such as mentioned XDP_FLAGS_UPDATE_IF_NOEXIST (or any other future
>> flags that are only relevant for loading, but never for dumping).
>> Given dumping IFLA_XDP_FLAGS was added due to XDP_FLAGS_SKB_MODE,
>> we can still change this, since it's not too late.
>>
>> How about the following proposal: IFLA_XDP_ATTACHED we have as-is
>> (need to keep that anyway), if that is true, it means "something
>> is attached at XDP layer". Then, we add a new attribute IFLA_XDP_MODE
>> (enum as type), which can contain XDP_DRV_MODE (0), XDP_SKB_MODE (1).
>> I don't think there's a strict requirement to really dump IFLA_XDP_FLAGS
>> back, separating both attrs would avoid this hassle of what current
>> or future load flag fits for dump as well and which not. Wdyt?
>
> I really like the idea of not reusing IFLA_XDP_FLAGS for dumps! New
> enum sounds good, but perhaps reusing IFLA_XDP_ATTACHED isn't 100%
> off-limits either? 4.11 would report (0) - driver supports XDP but
> nothing is attached, (1) - something attached at the driver, could we
> make (2) mean - something attached at generic XDP? Would that be
> considered breaking the ABI, are we bound to boolean semantics?
I like the idea, it would also render IFLA_XDP_ATTACHED not useless
or redundant then. Older binaries check !rta_getattr_u8(tb[IFLA_XDP_ATTACHED])
whether something is attached or not at XDP layer. So they won't know
IFLA_XDP_FLAGS attr either to make a more fine-grained distinction about
what mode. That seems actually cleaner to me, will give it a try.
Thanks,
Daniel
prev parent reply other threads:[~2017-05-10 23:02 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-10 1:31 [PATCH net 0/2] Two generic xdp related follow-ups Daniel Borkmann
2017-05-10 1:31 ` [PATCH net 1/2] xdp: add flag to enforce driver mode Daniel Borkmann
2017-05-10 1:31 ` [PATCH net 2/2] xdp: disallow use of native and generic hook at once Daniel Borkmann
2017-05-10 3:18 ` Jakub Kicinski
2017-05-10 9:36 ` Daniel Borkmann
2017-05-10 21:07 ` Jakub Kicinski
2017-05-10 22:24 ` Daniel Borkmann
2017-05-10 22:46 ` Jakub Kicinski
2017-05-10 23:02 ` Daniel Borkmann [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=59139BF0.5090606@iogearbox.net \
--to=daniel@iogearbox$(echo .)net \
--cc=alexei.starovoitov@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=john.fastabend@gmail$(echo .)com \
--cc=kubakici@wp$(echo .)pl \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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