| Message ID | 20191217084718.52098-1-dust.li@linux.alibaba.com |
|---|---|
| Headers | show |
| Series | net: sched: unify __gnet_stats_copy_xxx() for percpu and non-percpu | expand |
From: Dust Li <dust.li@linux.alibaba.com> Date: Tue, 17 Dec 2019 16:47:16 +0800 > Currently, __gnet_stats_copy_xxx() will overwrite the return value when > percpu stats are not enabled. But when percpu stats are enabled, it will > add the percpu stats to the result. This inconsistency brings confusion to > its callers. > > This patch series unify the behaviour of __gnet_stats_copy_basic() and > __gnet_stats_copy_queue() for percpu and non-percpu stats and fix an > incorrect statistic for mqprio class. > > - Patch 1 unified __gnet_stats_copy_xxx() for both percpu and non-percpu > - Patch 2 depending on Patch 1, fixes the problem that 'tc class show' > for mqprio class is always 0. I think this is going to break the estimator. The callers of est_fetch_counters() expect continually incrementing statistics. It does relative subtractions from previous values each time, so if we just reset on the next statistics fetch it won't work. Sorry I can't apply this.
On 12/21/19 8:54 AM, David Miller wrote: > From: Dust Li <dust.li@linux.alibaba.com> > Date: Tue, 17 Dec 2019 16:47:16 +0800 > >> Currently, __gnet_stats_copy_xxx() will overwrite the return value when >> percpu stats are not enabled. But when percpu stats are enabled, it will >> add the percpu stats to the result. This inconsistency brings confusion to >> its callers. >> >> This patch series unify the behaviour of __gnet_stats_copy_basic() and >> __gnet_stats_copy_queue() for percpu and non-percpu stats and fix an >> incorrect statistic for mqprio class. >> >> - Patch 1 unified __gnet_stats_copy_xxx() for both percpu and non-percpu >> - Patch 2 depending on Patch 1, fixes the problem that 'tc class show' >> for mqprio class is always 0. > I think this is going to break the estimator. > > The callers of est_fetch_counters() expect continually incrementing > statistics. It does relative subtractions from previous values each > time, so if we just reset on the next statistics fetch it won't work. > > Sorry I can't apply this. Hi David, Thanks for your reply. I checked the callers of est_fetch_counters(). I think there is a little misunderstanding here.We memset() the 'gnet_stats_basic_packed *b'which is not the original data of the estimator,but just the return value. Actually, it has been already memseted in est_fetch_counters() now. So it shouldnot break the estimator. static void est_fetch_counters(struct net_rate_estimator *e, struct gnet_stats_basic_packed *b) { / memset(b, 0, sizeof(*b)); // <<--- Here b is already memseted/ if (e->stats_lock) spin_lock(e->stats_lock); __gnet_stats_copy_basic(e->running, b, e->cpu_bstats, e->bstats); if (e->stats_lock) spin_unlock(e->stats_lock); } The purpose of this memset() is to maintain a consistent semantics for both percpu and non-percpu statisticsin __gnet_stats_copy_basic(). Thanks Dust
Currently, __gnet_stats_copy_xxx() will overwrite the return value when percpu stats are not enabled. But when percpu stats are enabled, it will add the percpu stats to the result. This inconsistency brings confusion to its callers. This patch series unify the behaviour of __gnet_stats_copy_basic() and __gnet_stats_copy_queue() for percpu and non-percpu stats and fix an incorrect statistic for mqprio class. - Patch 1 unified __gnet_stats_copy_xxx() for both percpu and non-percpu - Patch 2 depending on Patch 1, fixes the problem that 'tc class show' for mqprio class is always 0. Dust Li (2): net: sched: keep __gnet_stats_copy_xxx() same semantics for percpu stats net: sched: fix wrong class stats dumping in sch_mqprio net/core/gen_stats.c | 2 ++ net/sched/sch_mq.c | 35 ++++++++++++------------- net/sched/sch_mqprio.c | 59 +++++++++++++++++++++++------------------- 3 files changed, 51 insertions(+), 45 deletions(-)