On Thu, 7 Sep 2017 16:05:03 +0200, Jiri Pirko wrote: > Thu, Sep 07, 2017 at 03:44:12PM CEST, kubak...@wp.pl wrote: > >On Thu, 7 Sep 2017 11:10:33 +0200, Jiri Pirko wrote: > >> Hi Kuba. > >> > >> I'm looking into cls_bpf code and nfp_net_bpf_offload function in your > >> driver. Why do you need TC_CLSBPF_ADD? Seems like TC_CLSBPF_REPLACE > >> should be enough. It would make the cls_bpf code easier. > >> > >> Note that other cls just have replace/destroy (u32 too, as drivers > >> handle NEW/REPLACE in one switch-case - will patch this). > > > >Could we clarify what the REPLACE is actually supposed to do? :) > > > >In the flower code and the REPLACE looks a lot like ADD on the > >surface... If change is called it will invoke REPLACE with the new > >filter and then if there was an old filter, it will do DELETE. Is my > >understanding correct? > > Yes, correct. > > > > >If so I found this model of operation somehow confusing. Plus the > >management of flows may get slightly tricky if there is a possibility of > >"replacing" a flow with an identical one. Flower may make calls like > >these: > > > >add flower vlan_id 100 action ... > ># REPLACE vid 100 ... > >change ... flower vlan_id 100 action ... > ># REPLACE vid 100 ... > ># DELETE vid 100 ... > > Yes, that is the flow. > > > > >Doesn't this force driver/HW to implement refcounting on the rules? > > Why do you think so? There is a cookie that is passed from flower down > and driver uses it to remove the entry.
Right, the key/mask combination doesn't have to be unique anyway... > >On why I need the replace - BPF unlike other classifiers usually > >installs a single program, I think offloading multiple TC filters is > >questionable (people will use tailcalls instead most likely). I want to > >be able to implement atomic replace of that single program (i.e. not ADD > >followed by DELETE) because that simplifies the driver quite a bit. > > Understood. So, looks like the REPLACE/DESTROY would be sufficient for > bpf. ADD is not needed as it can be done by REPLACE-NULL, right? Yes, or you could take it to the extreme ;) DESTROY == offload(old, NULL) ADD == offload(NULL, new) REPLACE == offload(obj, new) > On the other hand, the rest of the cls, namely flower, u32 and matchall > need ADD/DESTROY as they don't really do no replacing. > > Makes sense? Ack, if you're unifying things, I don't mind how things are muxed as long as atomic replace is possible. FWIW cls_bpf doesn't pass old prog in REPLACE right now, but I have patch to add it anyway since it simplifies the driver when maps are involved. I should probably stop looking at the .command completely, just rely on new/old programs being populated.
