On Mon, Jun 14, 2021 at 03:02:07PM IST, Yaniv Agman wrote: > Hi Kartikeya, > > I recently started experimenting with the new tc-bpf API (which is > great, many thanks!) and I wanted to share a potential problem I > found. > I'm using this "Fixes for TC-BPF series" thread to write about it, but > it is not directly related to this patch set. > > According to the API summary given in > https://lore.kernel.org/bpf/20210512103451.989420-3-memxor@xxxxxxxxx/, > "It is advised that if the qdisc is operated on by many programs, > then the program at least check that there are no other existing > filters before deleting the clsact qdisc." > In the example given, one should: > > /* set opts as NULL, as we're not really interested in > * getting any info for a particular filter, but just > * detecting its presence. > */ > r = bpf_tc_query(&hook, NULL); > Yes, at some revision this worked, but then we changed it to not allow passing opts as NULL and I forgot to remove the snippet from the commit message. Sorry for that, but now it's buried in the git history forever :/. Mea Culpa. > However, following in this summary, where bpf_tc_query is described, > it is written that the opts argument cannot be NULL. > And indeed, when I tried to use the example above, an error (EINVAL) > was returned (as expected?) > > Am I missing something? > You are correct. We could do a few thing things: 1. Add a separate documentation file that correctly describes things (everything minus that para). 2. Support passing NULL to just detect presence of filters at a hook. 3. Add a multi query API that dumps all filters. Regardless of what we choose here, it will still be racy to clean up the qdisc a program installs itself, as there is a small race (but a race nonetheless) between checking of installed filters and removing the qdisc. I will discuss this today in the TC meeting to find some proper solution instead of the current hack. For now it would probably be best to leave it around I guess, though that does entail a small performance impact (due to enabling the sch_handle_{ingress,egress} static key). -- Kartikeya
