Question: how to detect if a qdisc is root or not?

Question: how to detect if a qdisc is root or not?

[Waskiewicz Jr, Peter P]

I've been tracking down an issue with the recent multiqueue code,
specifically with sch_prio and sch_rr loading as a root qdisc.  Right
now, we do not want to allow child qdiscs of sch_rr and sch_prio to load
with multiqueue enabled; we want to restrict multiqueue-enabled qdiscs
to the root qdisc (since this is the only thing to push into the
device).  The issue I have is I don't know how to detect if the qdisc
I'm currently processing is the root qdisc, or if it's a child.  From
sch_prio.c:

        q->mq = RTA_GET_FLAG(tb[TCA_PRIO_MQ - 1]);
        if (q->mq) {
                if (sch->handle != TC_H_ROOT)
                        return -EINVAL;

                if (netif_is_multiqueue(sch->dev)) { 

Unfortunately, this code isn't working.  This sch->handle is the handle
assigned to the qdisc upon creation, and it's not TC_H_ROOT.  Now the
information whether or not the qdisc is root gets passed via tc from
userspace, but that lives in the tcmsg struct, not the rtattr list of
options.  Does anyone know, outside of adding another attribute to the
rtattr list, how to detect if a qdisc is a root qdisc within the
prio_tune() initialization routine?  Any and all help would be much
appreciated.

Thanks,
 
PJ Waskiewicz
Intel Corporation
peter.p.waskiewicz.jr@intel.com <mailto:peter.p.waskiewicz.jr@intel.com>

Re: Question: how to detect if a qdisc is root or not?

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
> I've been tracking down an issue with the recent multiqueue code,
> specifically with sch_prio and sch_rr loading as a root qdisc.  Right
> now, we do not want to allow child qdiscs of sch_rr and sch_prio to load
> with multiqueue enabled; we want to restrict multiqueue-enabled qdiscs
> to the root qdisc (since this is the only thing to push into the
> device).  The issue I have is I don't know how to detect if the qdisc
> I'm currently processing is the root qdisc, or if it's a child.  From
> sch_prio.c:
> 
>         q->mq = RTA_GET_FLAG(tb[TCA_PRIO_MQ - 1]);
>         if (q->mq) {
>                 if (sch->handle != TC_H_ROOT)
>                         return -EINVAL;
> 
>                 if (netif_is_multiqueue(sch->dev)) { 
> 
> Unfortunately, this code isn't working.  This sch->handle is the handle
> assigned to the qdisc upon creation, and it's not TC_H_ROOT.


You're right, thats a bug. TC_H_ROOT is the parent ID, which is
stored in sch->parent. IIRC its also passed to the ->init() function.

RE: Question: how to detect if a qdisc is root or not?

[Waskiewicz Jr, Peter P]

> You're right, thats a bug. TC_H_ROOT is the parent ID, which 
> is stored in sch->parent. IIRC its also passed to the 
> ->init() function.

Unfortunately it's not passed.  It is passed into the ->change()
function:

static int prio_init(struct Qdisc *sch, struct rtattr *opt)

static int prio_change(struct Qdisc *sch, u32 handle, u32 parent, struct
rtattr **tca, unsigned long *arg)

I did mess around with sch->parent a bit, with no success (it appears to
be zero / unitialized).  I'll keep investigating.

Thanks,

-PJ Waskiewicz

Re: Question: how to detect if a qdisc is root or not?

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>>You're right, thats a bug. TC_H_ROOT is the parent ID, which 
>>is stored in sch->parent. IIRC its also passed to the 
>>->init() function.
> 
> 
> Unfortunately it's not passed.  It is passed into the ->change()
> function:
> 
> static int prio_init(struct Qdisc *sch, struct rtattr *opt)
> 
> static int prio_change(struct Qdisc *sch, u32 handle, u32 parent, struct
> rtattr **tca, unsigned long *arg)
> 
> I did mess around with sch->parent a bit, with no success (it appears to
> be zero / unitialized).  I'll keep investigating.


Its set after grafting the parent, which is after initialization.
I think what should work is to set it in qdisc_create instead,
sch_api.c around line 490:

+	sch->parent = handle;

        if (handle == TC_H_INGRESS) {
                sch->flags |= TCQ_F_INGRESS;
                sch->stats_lock = &dev->ingress_lock;
...

and remove the initialization in qdisc_graft. That would additionally
have the benefit that ingress qdiscs also have it initialized properly.

RE: Question: how to detect if a qdisc is root or not?

[Waskiewicz Jr, Peter P]

> Its set after grafting the parent, which is after initialization.
> I think what should work is to set it in qdisc_create 
> instead, sch_api.c around line 490:
> 
> +	sch->parent = handle;
> 
>         if (handle == TC_H_INGRESS) {
>                 sch->flags |= TCQ_F_INGRESS;
>                 sch->stats_lock = &dev->ingress_lock; ...
> 
> and remove the initialization in qdisc_graft. That would 
> additionally have the benefit that ingress qdiscs also have 
> it initialized properly.

That's where I'm messing around right now, and I did see that.  Let me
put it in and test.  If all is well, I'll post a patch.

Thanks for the help Patrick.

-PJ Waskiewicz

RE: Question: how to detect if a qdisc is root or not?

[Waskiewicz Jr, Peter P]

> Its set after grafting the parent, which is after initialization.
> I think what should work is to set it in qdisc_create 
> instead, sch_api.c around line 490:
> 
> +	sch->parent = handle;
> 
>         if (handle == TC_H_INGRESS) {
>                 sch->flags |= TCQ_F_INGRESS;
>                 sch->stats_lock = &dev->ingress_lock; ...
> 
> and remove the initialization in qdisc_graft. That would 
> additionally have the benefit that ingress qdiscs also have 
> it initialized properly.

I just sent out a patch to fix this.  Sorry for the delay; my
development machine oops'd in the middle of some disk I/O, and it
corrupted part of the inode table...the ext3 journal application seemed
to make it worse too.  Rebuilt the machine, so I'm back on my feet.

Anyways, I tried a few different things, and what it looks like is
sch->parent will be NULL (0) for the top-level device.  This is correct,
and trying to mess with that screws up qdisc_graft() when unloading the
qdisc.  I also tried adding a TCQ_F_ROOT flag to sch->flags when classid
is TC_H_ROOT, but that also screwed up unloading the qdisc.

The ingress qdisc does have the parent handle set correctly, namely
NULL, since it will always be the top-level qdisc on ingress sessions.

Thx Patrick,
-PJ Waskiewicz

Re: Question: how to detect if a qdisc is root or not?

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>>Its set after grafting the parent, which is after initialization.
>>I think what should work is to set it in qdisc_create 
>>instead, sch_api.c around line 490:
>>
>>+	sch->parent = handle;
>>
>>        if (handle == TC_H_INGRESS) {
>>                sch->flags |= TCQ_F_INGRESS;
>>                sch->stats_lock = &dev->ingress_lock; ...
>>
>>and remove the initialization in qdisc_graft. That would 
>>additionally have the benefit that ingress qdiscs also have 
>>it initialized properly.
> 
> 
> I just sent out a patch to fix this.


I didn't see it yet.

> Sorry for the delay; my
> development machine oops'd in the middle of some disk I/O, and it
> corrupted part of the inode table...the ext3 journal application seemed
> to make it worse too.  Rebuilt the machine, so I'm back on my feet.


No worries :)

> Anyways, I tried a few different things, and what it looks like is
> sch->parent will be NULL (0) for the top-level device.  This is correct,
> and trying to mess with that screws up qdisc_graft() when unloading the
> qdisc.  I also tried adding a TCQ_F_ROOT flag to sch->flags when classid
> is TC_H_ROOT, but that also screwed up unloading the qdisc.


I dont think I understand. Whats the problem with setting sch->parent
on initialization instead on grafting as I did in my example patch?
Please explain the problems arrising on unload in detail.

Re: Question: how to detect if a qdisc is root or not?

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Sat, 21 Jul 2007 06:01:07 +0200

> Waskiewicz Jr, Peter P wrote:
> > I just sent out a patch to fix this.
> 
> I didn't see it yet.

VGER ate it for some reason and I didn't notice it during my
daily bounce analysis.  I asked him on IRC to resend it, but
he's away from the computer already :)

RE: Question: how to detect if a qdisc is root or not?

[Waskiewicz Jr, Peter P]

> From: Patrick McHardy <kaber@trash.net>
> Date: Sat, 21 Jul 2007 06:01:07 +0200
> 
> > Waskiewicz Jr, Peter P wrote:
> > > I just sent out a patch to fix this.
> > 
> > I didn't see it yet.
> 
> VGER ate it for some reason and I didn't notice it during my 
> daily bounce analysis.  I asked him on IRC to resend it, but 
> he's away from the computer already :)

I'll get it resent today.  Sorry for the delay.

-PJ

RE: Question: how to detect if a qdisc is root or not?

[Waskiewicz Jr, Peter P]

> > Anyways, I tried a few different things, and what it looks like is
> > sch->parent will be NULL (0) for the top-level device.  This is 
> > sch->correct,
> > and trying to mess with that screws up qdisc_graft() when unloading 
> > the qdisc.  I also tried adding a TCQ_F_ROOT flag to 
> sch->flags when 
> > classid is TC_H_ROOT, but that also screwed up unloading the qdisc.
> 
> 
> I dont think I understand. Whats the problem with setting 
> sch->parent on initialization instead on grafting as I did in 
> my example patch?
> Please explain the problems arrising on unload in detail.

sch->parent is getting set on initialization, and for the root and
ingress qdiscs, it's left at zero.  If I change that value, when the
root qdisc is unloaded and pfifo_fast is put back into place, the
qdisc_destroy() walks the tree and attempts to free memory from the
handle pointed at by sch->parent.  It stops when sch->parent is NULL, so
sch->parent is actually being set as intended.  The only thing that
confused me is that nowhere in the qdisc is TC_H_ROOT included
explicitly, rather, the root qdisc is where sch->parent is NULL.

So I misunderstood what was actually wrong.  The qdisc code is ok as-is,
it's just that the top-level qdisc (root and ingress) have a sch->parent
of NULL, which is being set correctly today.

Hope that clarifies.

Thanks,
-PJ

Re: Question: how to detect if a qdisc is root or not?

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>>I dont think I understand. Whats the problem with setting 
>>sch->parent on initialization instead on grafting as I did in 
>>my example patch?
>>Please explain the problems arrising on unload in detail.
> 
> 
> sch->parent is getting set on initialization, and for the root and
> ingress qdiscs, it's left at zero.  If I change that value, when the
> root qdisc is unloaded and pfifo_fast is put back into place, the
> qdisc_destroy() walks the tree and attempts to free memory from the
> handle pointed at by sch->parent.


First of all, qdisc destruction never propagates up, only down
the tree. Secondly neither qdisc_destroy nor pfifo nor prio
even look at sch->parent. So this is completely wrong, the only
place where sch->parent is used for walking through the tree
is qdisc_tree_decrease_qlen.

> It stops when sch->parent is NULL,

Where are you getting this? sch->parent is an *integer*.

> so
> sch->parent is actually being set as intended.  The only thing that
> confused me is that nowhere in the qdisc is TC_H_ROOT included
> explicitly, rather, the root qdisc is where sch->parent is NULL.
> 
> So I misunderstood what was actually wrong.  The qdisc code is ok as-is,
> it's just that the top-level qdisc (root and ingress) have a sch->parent
> of NULL, which is being set correctly today.
> 
> Hope that clarifies.

Not at all :)