| Message ID | 20201007165111.172419-1-eric.dumazet@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net/sched: get rid of qdisc->padded | expand |
On Wed, Oct 7, 2020 at 9:51 AM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > From: Eric Dumazet <edumazet@google.com> > > kmalloc() of sufficiently big portion of memory is cache-aligned > in regular conditions. If some debugging options are used, > there is no reason qdisc structures would need 64-byte alignment > if most other kernel structures are not aligned. > > This get rid of QDISC_ALIGN and QDISC_ALIGNTO. > > Addition of privdata field will help implementing > the reverse of qdisc_priv() and documents where > the private data is. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Cong Wang <xiyou.wangcong@gmail.com> > Cc: Allen Pais <allen.lkml@gmail.com> Acked-by: Cong Wang <xiyou.wangcong@gmail.com> Thanks.
On Wed, 7 Oct 2020 09:51:11 -0700 Eric Dumazet wrote: > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h > index 6c762457122fd0091cb0f2bf41bda73babc4ac12..d8fd8676fc724110630904909f64d7789f3a4b47 100644 > --- a/include/net/sch_generic.h > +++ b/include/net/sch_generic.h > @@ -91,7 +91,7 @@ struct Qdisc { > struct net_rate_estimator __rcu *rate_est; > struct gnet_stats_basic_cpu __percpu *cpu_bstats; > struct gnet_stats_queue __percpu *cpu_qstats; > - int padded; > + int pad; > refcount_t refcnt; > > /* Hi Eric! Why keep the pad field? the member to lines down is __cacheline_aligned, so we shouldn't have to manually push things out? struct gnet_stats_queue __percpu *cpu_qstats; int pad; refcount_t refcnt; /* * For performance sake on SMP, we put highly modified fields at the end */ struct sk_buff_head gso_skb ____cacheline_aligned_in_smp;
On Fri, Oct 9, 2020 at 2:18 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 7 Oct 2020 09:51:11 -0700 Eric Dumazet wrote: > > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h > > index 6c762457122fd0091cb0f2bf41bda73babc4ac12..d8fd8676fc724110630904909f64d7789f3a4b47 100644 > > --- a/include/net/sch_generic.h > > +++ b/include/net/sch_generic.h > > @@ -91,7 +91,7 @@ struct Qdisc { > > struct net_rate_estimator __rcu *rate_est; > > struct gnet_stats_basic_cpu __percpu *cpu_bstats; > > struct gnet_stats_queue __percpu *cpu_qstats; > > - int padded; > > + int pad; > > refcount_t refcnt; > > > > /* > > Hi Eric! > > Why keep the pad field? the member to lines down is > __cacheline_aligned, so we shouldn't have to manually > push things out? > > struct gnet_stats_queue __percpu *cpu_qstats; > int pad; I usually prefer adding explicit fields to tell where the holes are, for future reuse. Not many developers are aware of pahole existence :/ I renamed it to pad, I could have used padding or something else.
On Fri, 9 Oct 2020 09:35:03 +0200 Eric Dumazet wrote: > On Fri, Oct 9, 2020 at 2:18 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Wed, 7 Oct 2020 09:51:11 -0700 Eric Dumazet wrote: > > > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h > > > index 6c762457122fd0091cb0f2bf41bda73babc4ac12..d8fd8676fc724110630904909f64d7789f3a4b47 100644 > > > --- a/include/net/sch_generic.h > > > +++ b/include/net/sch_generic.h > > > @@ -91,7 +91,7 @@ struct Qdisc { > > > struct net_rate_estimator __rcu *rate_est; > > > struct gnet_stats_basic_cpu __percpu *cpu_bstats; > > > struct gnet_stats_queue __percpu *cpu_qstats; > > > - int padded; > > > + int pad; > > > refcount_t refcnt; > > > > > > /* > > > > Hi Eric! > > > > Why keep the pad field? the member to lines down is > > __cacheline_aligned, so we shouldn't have to manually > > push things out? > > > > struct gnet_stats_queue __percpu *cpu_qstats; > > int pad; > > I usually prefer adding explicit fields to tell where the holes are, > for future reuse. > Not many developers are aware of pahole existence :/ > I renamed it to pad, I could have used padding or something else. Alright then, applied, thank you!
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index ac8c890a2657e35546ec51fe8b8a993a2bd0c91b..4ed32e6b020145afb015c3c07d2ec3a613f1311d 100644 --- a/include/net/pkt_sched.h +++ b/include/net/pkt_sched.h @@ -19,12 +19,9 @@ struct qdisc_walker { int (*fn)(struct Qdisc *, unsigned long cl, struct qdisc_walker *); }; -#define QDISC_ALIGNTO 64 -#define QDISC_ALIGN(len) (((len) + QDISC_ALIGNTO-1) & ~(QDISC_ALIGNTO-1)) - static inline void *qdisc_priv(struct Qdisc *q) { - return (char *) q + QDISC_ALIGN(sizeof(struct Qdisc)); + return &q->privdata; } /* diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 6c762457122fd0091cb0f2bf41bda73babc4ac12..d8fd8676fc724110630904909f64d7789f3a4b47 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -91,7 +91,7 @@ struct Qdisc { struct net_rate_estimator __rcu *rate_est; struct gnet_stats_basic_cpu __percpu *cpu_bstats; struct gnet_stats_queue __percpu *cpu_qstats; - int padded; + int pad; refcount_t refcnt; /* @@ -112,6 +112,9 @@ struct Qdisc { /* for NOLOCK qdisc, true if there are no enqueued skbs */ bool empty; struct rcu_head rcu; + + /* private data */ + long privdata[] ____cacheline_aligned; }; static inline void qdisc_refcount_inc(struct Qdisc *qdisc) diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 54c417244642a1445c618e6adf0e38b2d4f84565..49eae93d1489dc3513b41c237ca3f572e21ff203 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -802,9 +802,8 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue, const struct Qdisc_ops *ops, struct netlink_ext_ack *extack) { - void *p; struct Qdisc *sch; - unsigned int size = QDISC_ALIGN(sizeof(*sch)) + ops->priv_size; + unsigned int size = sizeof(*sch) + ops->priv_size; int err = -ENOBUFS; struct net_device *dev; @@ -815,22 +814,10 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue, } dev = dev_queue->dev; - p = kzalloc_node(size, GFP_KERNEL, - netdev_queue_numa_node_read(dev_queue)); + sch = kzalloc_node(size, GFP_KERNEL, netdev_queue_numa_node_read(dev_queue)); - if (!p) + if (!sch) goto errout; - sch = (struct Qdisc *) QDISC_ALIGN((unsigned long) p); - /* if we got non aligned memory, ask more and do alignment ourself */ - if (sch != p) { - kfree(p); - p = kzalloc_node(size + QDISC_ALIGNTO - 1, GFP_KERNEL, - netdev_queue_numa_node_read(dev_queue)); - if (!p) - goto errout; - sch = (struct Qdisc *) QDISC_ALIGN((unsigned long) p); - sch->padded = (char *) sch - (char *) p; - } __skb_queue_head_init(&sch->gso_skb); __skb_queue_head_init(&sch->skb_bad_txq); qdisc_skb_head_init(&sch->q); @@ -873,7 +860,7 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue, return sch; errout1: - kfree(p); + kfree(sch); errout: return ERR_PTR(err); } @@ -941,7 +928,7 @@ void qdisc_free(struct Qdisc *qdisc) free_percpu(qdisc->cpu_qstats); } - kfree((char *) qdisc - qdisc->padded); + kfree(qdisc); } static void qdisc_free_cb(struct rcu_head *head)