lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20101003094221.GA2028@del.dom.local> Date: Sun, 3 Oct 2010 11:42:21 +0200 From: Jarek Poplawski <jarkao2@...il.com> To: Eric Dumazet <eric.dumazet@...il.com> Cc: hadi@...erus.ca, David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org> Subject: Re: [PATCH net-next V3] net: dynamic ingress_queue allocation 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@...il.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... ;-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists