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
