* [PATCH net-next] net/sched: get rid of qdisc->padded
@ 2020-10-07 16:51 Eric Dumazet
2020-10-08 1:42 ` Cong Wang
2020-10-09 0:18 ` Jakub Kicinski
0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2020-10-07 16:51 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Eric Dumazet, Cong Wang, Allen Pais
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>
---
include/net/pkt_sched.h | 5 +----
include/net/sch_generic.h | 5 ++++-
net/sched/sch_generic.c | 23 +++++------------------
3 files changed, 10 insertions(+), 23 deletions(-)
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)
--
2.28.0.806.g8561365e88-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net/sched: get rid of qdisc->padded
2020-10-07 16:51 [PATCH net-next] net/sched: get rid of qdisc->padded Eric Dumazet
@ 2020-10-08 1:42 ` Cong Wang
2020-10-09 0:18 ` Jakub Kicinski
1 sibling, 0 replies; 5+ messages in thread
From: Cong Wang @ 2020-10-08 1:42 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, Allen Pais
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net/sched: get rid of qdisc->padded
2020-10-07 16:51 [PATCH net-next] net/sched: get rid of qdisc->padded Eric Dumazet
2020-10-08 1:42 ` Cong Wang
@ 2020-10-09 0:18 ` Jakub Kicinski
2020-10-09 7:35 ` Eric Dumazet
1 sibling, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2020-10-09 0:18 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, netdev, Eric Dumazet, Cong Wang, Allen Pais
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;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net/sched: get rid of qdisc->padded
2020-10-09 0:18 ` Jakub Kicinski
@ 2020-10-09 7:35 ` Eric Dumazet
2020-10-09 15:38 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2020-10-09 7:35 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Eric Dumazet, David S . Miller, netdev, Cong Wang, Allen Pais
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net/sched: get rid of qdisc->padded
2020-10-09 7:35 ` Eric Dumazet
@ 2020-10-09 15:38 ` Jakub Kicinski
0 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2020-10-09 15:38 UTC (permalink / raw)
To: Eric Dumazet
Cc: Eric Dumazet, David S . Miller, netdev, Cong Wang, Allen Pais
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!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-09 15:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 16:51 [PATCH net-next] net/sched: get rid of qdisc->padded Eric Dumazet
2020-10-08 1:42 ` Cong Wang
2020-10-09 0:18 ` Jakub Kicinski
2020-10-09 7:35 ` Eric Dumazet
2020-10-09 15:38 ` Jakub Kicinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.