Re: [Patch net-next] net_sched: fix an extack message in tcf_block_find()

[Cong Wang <xiyou.wangcong@gmail.com>]

On Thu, Sep 27, 2018 at 2:16 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 09/27/2018 01:42 PM, Cong Wang wrote:
> > It is clearly a copy-n-paste.
> >
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > ---
> >  net/sched/cls_api.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> > index 3de47e99b788..8dd7f8af6d54 100644
> > --- a/net/sched/cls_api.c
> > +++ b/net/sched/cls_api.c
> > @@ -655,7 +655,7 @@ static struct tcf_block *tcf_block_find(struct net *net, struct Qdisc **q,
> >
> >               *q = qdisc_refcount_inc_nz(*q);
> >               if (!*q) {
> > -                     NL_SET_ERR_MSG(extack, "Parent Qdisc doesn't exists");
> > +                     NL_SET_ERR_MSG(extack, "Can't increase Qdisc refcount");
>
>
> I am not sure it was a copy-n-paste.


Make sure you knew there is an exactly same extack message
(with a same English grammar error).


>
> Qdisc refcount business is kernel internal.

Yeah, but the extack message is already there, this patch doesn't add
any new extack. Or you are suggesting we should remove it?



> If we can not increase the refcount, this is precisely because this qdisc is about
> to be destroyed. Nothing fundamentally different than having this thread delayed a bit
> and qdisc_lookup_rcu() returning NULL in the first place.


qdisc_lookup_rcu() is not always called, it could be dev->qdisc.
I am pretty sure parent exists in dev->qdisc.


>
> This also means that using RCU for control path is problematic, as surely the caller
> of this interface would prefer something that succeeds, even if this means
> waiting a bit in the kernel.

I fail to validate this statement, Why it prefers success when refcnt reaches
0?


>
> Or are we willing to change ip command and make it restart failed syscalls ?
>

I don't understand what you mean by changing ip command, you must
mean tc command, but still, I have no idea about how restarting failed
syscall could be related to my patch and why we need to restart anything
here. If the refcnt goes to 0, it will never come back, retrying won't help
anything.

BTW:

If you have any other question beyond my patch's scope, isn't it better
that we start a new thread for discussion?

In case you still misunderstand, my patch never intends to address any
other problem rather than correcting an inaccurate extack message.