From: Jarek Poplawski <jarkao2@gmail•com>
To: Eric Dumazet <eric.dumazet@gmail•com>
Cc: hadi@cyberus•ca, David Miller <davem@davemloft•net>,
netdev <netdev@vger•kernel.org>
Subject: Re: [PATCH net-next V3] net: dynamic ingress_queue allocation
Date: Sun, 3 Oct 2010 11:42:21 +0200 [thread overview]
Message-ID: <20101003094221.GA2028@del.dom.local> (raw)
In-Reply-To: <1286035915.2582.2472.camel@edumazet-laptop>
On Sat, Oct 02, 2010 at 06:11:55PM +0200, Eric Dumazet wrote:
> Le samedi 02 octobre 2010 ?? 11:32 +0200, Jarek Poplawski a écrit :
> > On Fri, Oct 01, 2010 at 03:56:28PM +0200, Eric Dumazet wrote:
...
> > > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > > index b802078..8635110 100644
> > > --- a/net/sched/sch_api.c
> > > +++ b/net/sched/sch_api.c
...
> > > @@ -690,6 +693,8 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
> > > (new && new->flags & TCQ_F_INGRESS)) {
> > > num_q = 1;
> > > ingress = 1;
> > > + if (!dev_ingress_queue(dev))
> > > + return -ENOENT;
> >
> > Is this test really needed here?
>
> To avoid a NULL dereference some lines later.
> Do I have a guarantee its not NULL here ?
Do you have any scenario for NULL here? ;-)
Of course, it's your patch and responsibility, and I'll not guarantee,
but you could at least add a TODO comment, to check it later.
> > > @@ -1044,7 +1050,8 @@ replay:
> > > return -ENOENT;
> > > q = qdisc_leaf(p, clid);
> > > } else { /*ingress */
> > > - q = dev->ingress_queue.qdisc_sleeping;
> > > + if (dev_ingress_queue_create(dev))
> > > + q = dev_ingress_queue(dev)->qdisc_sleeping;
> >
> > I wonder if doing dev_ingress_queue_create() just before qdisc_create()
> > (and the test here) isn't more readable.
>
> Sorry, I dont understand. I want to create ingress_queue only if user
> wants it. If we setup (egress) trafic shaping, no need to setup
> ingress_queue.
I mean doing both creates in one place:
> @@ -1123,11 +1130,14 @@ replay:
> create_n_graft:
...
> + if (clid == TC_H_INGRESS) {
+ if (dev_ingress_queue_create(dev))
> + q = qdisc_create(dev, dev_ingress_queue(dev), p,
> + tcm->tcm_parent, tcm->tcm_parent,
> + tca, &err);
> + else
> + err = -ENOENT;
> + } else {
> struct netdev_queue *dev_queue;
...
> Here is the V3 then.
>
> [PATCH net-next V3] net: dynamic ingress_queue allocation
>
> ingress being not used very much, and net_device->ingress_queue being
> quite a big object (128 or 256 bytes), use a dynamic allocation if
> needed (tc qdisc add dev eth0 ingress ...)
>
> dev_ingress_queue(dev) helper should be used only with RTNL taken.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail•com>
> ---
> V3: add rcu notations & address Jarek comments
> include/linux/netdevice.h | 2 -
> include/linux/rtnetlink.h | 8 ++++++
> net/core/dev.c | 34 ++++++++++++++++++++++-------
> net/sched/sch_api.c | 42 ++++++++++++++++++++++++------------
> net/sched/sch_generic.c | 12 ++++++----
> 5 files changed, 71 insertions(+), 27 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ceed347..92d81ed 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -986,7 +986,7 @@ struct net_device {
> rx_handler_func_t *rx_handler;
> void *rx_handler_data;
>
> - struct netdev_queue ingress_queue; /* use two cache lines */
> + struct netdev_queue __rcu *ingress_queue;
>
> /*
> * Cache lines mostly used on transmit path
> diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
> index 68c436b..0bb7b48 100644
> --- a/include/linux/rtnetlink.h
> +++ b/include/linux/rtnetlink.h
> @@ -6,6 +6,7 @@
> #include <linux/if_link.h>
> #include <linux/if_addr.h>
> #include <linux/neighbour.h>
> +#include <linux/netdevice.h>
>
> /* rtnetlink families. Values up to 127 are reserved for real address
> * families, values above 128 may be used arbitrarily.
> @@ -769,6 +770,13 @@ extern int lockdep_rtnl_is_held(void);
> #define rtnl_dereference(p) \
> rcu_dereference_check(p, lockdep_rtnl_is_held())
>
> +static inline struct netdev_queue *dev_ingress_queue(struct net_device *dev)
> +{
> + return rtnl_dereference(dev->ingress_queue);
I'd consider rcu_dereference_rtnl(). Btw, technically qdisc_lookup()
doesn't require rtnl, and there was time it was used without it
(on xmit path).
I think you should also add a comment here why this rcu is used, and
that it changes only once in dev's liftime.
Jarek P.
PS: checkpatched or not checkpatched, that is the question... ;-)
next prev parent reply other threads:[~2010-10-03 9:42 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-09-28 15:58 [PATCH net-next2.6] net: rename netdev rx_queue to ingress_queue Eric Dumazet
2010-09-28 18:04 ` Jarek Poplawski
2010-09-29 10:56 ` jamal
2010-09-29 13:18 ` Jarek Poplawski
2010-09-29 16:54 ` Jarek Poplawski
2010-09-30 22:58 ` [PATCH net-next2.6] net: dynamic ingress_queue allocation Eric Dumazet
2010-10-01 11:45 ` jamal
2010-10-01 13:56 ` [PATCH net-next V2] " Eric Dumazet
2010-10-02 9:32 ` Jarek Poplawski
2010-10-02 16:11 ` [PATCH net-next V3] " Eric Dumazet
2010-10-03 9:42 ` Jarek Poplawski [this message]
2010-10-03 13:10 ` jamal
2010-10-04 8:42 ` Eric Dumazet
2010-10-04 9:00 ` [PATCH net-next] net: relax rtnl_dereference() Eric Dumazet
2010-10-05 7:29 ` David Miller
2010-10-04 12:06 ` [PATCH net-next V3] net: dynamic ingress_queue allocation Jarek Poplawski
2010-10-04 12:52 ` Eric Dumazet
2010-10-04 14:24 ` Jarek Poplawski
2010-10-04 15:21 ` Eric Dumazet
2010-10-05 7:24 ` David Miller
2010-10-05 7:31 ` Eric Dumazet
2010-10-02 12:10 ` [PATCH net-next V2] " jamal
2010-09-29 20:27 ` [PATCH net-next2.6] net: rename netdev rx_queue to ingress_queue David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20101003094221.GA2028@del.dom.local \
--to=jarkao2@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=hadi@cyberus$(echo .)ca \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox