Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1590659 > unrolled thread

Re: [PATCH 1/2] net: sched: make default fifo qdiscs appear in the dump

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] net: sched: make default fifo qdiscs appear in the  dump David Miller <davem@davemloft.net> - 2017-03-01 23:30 +0100
    Re: [PATCH 1/2] net: sched: make default fifo qdiscs appear in the  dump Jiri Kosina <jikos@kernel.org> - 2017-03-06 12:30 +0100
      Re: [PATCH 1/2] net: sched: make default fifo qdiscs appear in the  dump David Miller <davem@davemloft.net> - 2017-03-06 19:40 +0100
        Re: [PATCH 1/2] net: sched: make default fifo qdiscs appear in the  dump Jiri Kosina <jikos@kernel.org> - 2017-03-07 11:20 +0100

#1590659 — Re: [PATCH 1/2] net: sched: make default fifo qdiscs appear in the dump

From: Jiri Kosina <jikos@kernel.org>
Date: Sat, 25 Feb 2017 22:29:09 +0100 (CET)

> @@ -1066,6 +1066,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
>  				      &pfifo_qdisc_ops, classid);
>  	if (cl->qdisc == NULL)
>  		cl->qdisc = &noop_qdisc;
> +	qdisc_hash_add(cl->qdisc, true);
>  	INIT_LIST_HEAD(&cl->children);
>  	cl->vt_tree = RB_ROOT;
>  	cl->cf_tree = RB_ROOT;
> @@ -1425,6 +1426,7 @@ hfsc_init_qdisc(struct Qdisc *sch, struct nlattr *opt)
>  					  sch->handle);
>  	if (q->root.qdisc == NULL)
>  		q->root.qdisc = &noop_qdisc;
> +	qdisc_hash_add(q->root.qdisc, true);
>  	INIT_LIST_HEAD(&q->root.children);
>  	q->root.vt_tree = RB_ROOT;
>  	q->root.cf_tree = RB_ROOT;

I'm not so sure it is legal is potentially pass &noop_qdisc into qdisc_hash_add().

[toc] | [next] | [standalone]

#1593216

On Wed, 1 Mar 2017, David Miller wrote:

> > @@ -1066,6 +1066,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
> >  				      &pfifo_qdisc_ops, classid);
> >  	if (cl->qdisc == NULL)
> >  		cl->qdisc = &noop_qdisc;
> > +	qdisc_hash_add(cl->qdisc, true);
> >  	INIT_LIST_HEAD(&cl->children);
> >  	cl->vt_tree = RB_ROOT;
> >  	cl->cf_tree = RB_ROOT;
> > @@ -1425,6 +1426,7 @@ hfsc_init_qdisc(struct Qdisc *sch, struct nlattr *opt)
> >  					  sch->handle);
> >  	if (q->root.qdisc == NULL)
> >  		q->root.qdisc = &noop_qdisc;
> > +	qdisc_hash_add(q->root.qdisc, true);
> >  	INIT_LIST_HEAD(&q->root.children);
> >  	q->root.vt_tree = RB_ROOT;
> >  	q->root.cf_tree = RB_ROOT;
> 
> I'm not so sure it is legal is potentially pass &noop_qdisc into qdisc_hash_add().

Ah, right you are, thanks. The complete fix is not super trivial, as it 
needs some more surgery to tc_dump_qdisc_root(), tc_dump_tclass_root() and 
qdisc_match_from_root() (see 69012ae42 for some details).

There are two options:

- this gets fixed in two phases, in first everything *but* noop qdisc gets 
  dumped (in the "give me everything" dump) and later we finalize it by
  teaching the above functions about noop_qdisc as well

- I extend this patchset to handle noop qdisc from the very beginning; 
  I am unlikely to find time for this during coming weeks though. But OTOH
  this whole thing is very low priority anyway

What do you think?

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1593572

From: Jiri Kosina <jikos@kernel.org>
Date: Mon, 6 Mar 2017 12:03:38 +0100 (CET)

> Ah, right you are, thanks. The complete fix is not super trivial, as it 
> needs some more surgery to tc_dump_qdisc_root(), tc_dump_tclass_root() and 
> qdisc_match_from_root() (see 69012ae42 for some details).
> 
> There are two options:
> 
> - this gets fixed in two phases, in first everything *but* noop qdisc gets 
>   dumped (in the "give me everything" dump) and later we finalize it by
>   teaching the above functions about noop_qdisc as well
> 
> - I extend this patchset to handle noop qdisc from the very beginning; 
>   I am unlikely to find time for this during coming weeks though. But OTOH
>   this whole thing is very low priority anyway
> 
> What do you think?

I'm not too hot on this whole idea because the only way you can emit
the noop_qdisc is to "dup" it by allocating a new qdisc so that you
can link it in.  This has two downsides:

1) Extra overhead and memory usage

2) All of the simple checks against &noop_qdisc might not be
   so simply any more.

[toc] | [prev] | [next] | [standalone]

#1594077

On Mon, 6 Mar 2017, David Miller wrote:

> > Ah, right you are, thanks. The complete fix is not super trivial, as it 
> > needs some more surgery to tc_dump_qdisc_root(), tc_dump_tclass_root() and 
> > qdisc_match_from_root() (see 69012ae42 for some details).
> > 
> > There are two options:
> > 
> > - this gets fixed in two phases, in first everything *but* noop qdisc gets 
> >   dumped (in the "give me everything" dump) and later we finalize it by
> >   teaching the above functions about noop_qdisc as well
> > 
> > - I extend this patchset to handle noop qdisc from the very beginning; 
> >   I am unlikely to find time for this during coming weeks though. But OTOH
> >   this whole thing is very low priority anyway
> > 
> > What do you think?
> 
> I'm not too hot on this whole idea because the only way you can emit
> the noop_qdisc is to "dup" it by allocating a new qdisc so that you
> can link it in.  This has two downsides:
> 
> 1) Extra overhead and memory usage
> 
> 2) All of the simple checks against &noop_qdisc might not be
>    so simply any more.

Fully agreed.

I'd be inclined to just live with the fact that noop qdisc would never 
ever be visible in the dump. I think that in this particular case this can 
be easily justified, as the behavior is consistent with what is seen in 
the dump ("nothing happens to the packets"). All other cases will be 
covered.

If there are no objections to this, I'll send out v2 shortly.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web