On Fri, 8 Mar 2019 at 11:46, Maciej Fijalkowski <maciejromanfijalkowski@xxxxxxxxx> wrote: > [...] > > > > + flags = sxdp->sxdp_flags; > > > > + if (flags & ~(XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY)) > > > > + return -EINVAL; > > > > + > > > > > > What about setting more than one flag at a time? Is it allowed/make any sense? > > > After a quick look it seems that they exclude each other, e.g. you can't force > > > a zero copy and copy mode at the same time. And for XDP_SHARED_UMEM two > > > remaining flags can't be set. > > > > > > So maybe check here also that only one particular flag is set by doing: > > > > > > if (hweight32(flags & (XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY)) > 1) > > > return -EINVAL; > > > > > > just like we do it for IFLA_XDP_FLAGS in net/core/rtnetlink.c? > > > > > > > We have flag semantic checks further down, and my rational was to > > *only* check unknown flags first. IMO the current patch is easier to > > understand, than your suggested one. > > > > Hmm thought that bailing out earlier would be better and we could drop the > actual copy flags checks for shared umem. For xdp_umem_assign_dev() instead of > passing flags you could just pass a boolean whether you're doing zero copy or > not. And that brings up the question whether we really need a XDP_COPY flag? > I'd prefer doing that as a follow-up patch. XDP_COPY is needed to explicitly enable copy-mode. No flags is "select the best option", and COPY/ZEROCOPY is to explicitly select a mode.
