On Mon, 3 Jun 2019 at 19:03, Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx> wrote: > > On Mon, 3 Jun 2019 11:04:36 +0200, Björn Töpel wrote: > > On Sat, 1 Jun 2019 at 21:57, Jakub Kicinski > > <jakub.kicinski@xxxxxxxxxxxxx> wrote: > > > > > > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote: > > > > > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE) > > > > > + mode = XDP_FLAGS_SKB_MODE; > > > > > + > > > > > + curr_mode = dev_xdp_current_mode(dev); > > > > > + > > > > > + if (!offload && curr_mode && (mode ^ curr_mode) & > > > > > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) { > > > > > > > > if i am reading this correctly this is equivalent to : > > > > > > > > if (!offload && (curre_mode != mode)) > > > > offlad is false then curr_mode and mode must be DRV or GENERIC .. > > > > > > Naw, if curr_mode is not set, i.e. nothing installed now, we don't care > > > about the diff. > > > > > > > better if you keep bitwise operations for actual bitmasks, mode and > > > > curr_mode are not bitmask, they can hold one value each .. according to > > > > your logic.. > > > > > > Well, they hold one bit each, whether one bit is a bitmap perhaps is > > > disputable? :) > > > > > > I think the logic is fine. > > > > > > > Hmm, but changing to: > > > > if (!offload && curr_mode && mode != curr_mode) > > > > is equal, and to Saeed's point, clearer. I'll go that route in a v3. > > Sorry, you're right, the flags get mangled before they get here, so > yeah, this condition should work. Confusingly. > > > > What happened to my request to move the change in behaviour for > > > disabling to a separate patch, tho, Bjorn? :) > > > > Actually, I left that out completely. This patch doesn't change the > > behavior. After I realized how the flags *should* be used, I don't > > think my v1 change makes sense anymore. My v1 patch was to give an > > error if you tried to disable, say generic if drv was enabled via > > "auto detect/no flags". But this is catched by looking at the flags. > > > > What I did, however, was moving the flags check into change_fd so that > > the driver doesn't have to do the check. E.g. the Intel drivers didn't > > do correct checking of flags. > > Ugh. Could you please rewrite the conditions to make the fd >= check > consistently the outside if? Also could you add extack to this: > The reason I moved the if-statement (checking if we're mixing drv/skb), is because I'd like to catch the no-op (e.g. xdpdrv active and calling xdpgeneric off) early (the return 0, under the if (fd >= check). > + if (!offload && dev_xdp_query(dev, mode) && > + !xdp_prog_flags_ok(dev->xdp_flags, flags, extack)) > + return -EBUSY; > > It's unclear what it's doing. This checks whether the flags have changed, pulling out the logic from the drivers. xdp_prog_flags_ok adds to extack, resuing the flags_ok function. The xdp_attachment_flags_ok OTOH is not necessary anymore, and should be further cleaned up. I'll address this and make the this clause more clear. Björn