netdev - Re: [PATCH] net: sched: keep __gnet_stats_copy_xxx() same semantics for percpu stats

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpVYS9Am6G46iiNhg_OAft_=CLd5ziAFsMKt8sLmhuMCnQ@mail.gmail.com>
Date:   Fri, 29 Nov 2019 21:44:52 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Dust Li <dust.li@...ux.alibaba.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>,
        John Fastabend <john.fastabend@...il.com>,
        Tony Lu <tonylu@...ux.alibaba.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: sched: keep __gnet_stats_copy_xxx() same semantics
 for percpu stats

On Wed, Nov 27, 2019 at 10:31 PM Dust Li <dust.li@...ux.alibaba.com> wrote:
>
> __gnet_stats_copy_basic/queue() support both percpu stat and
> non-percpu stat, but they are handle in a different manner:
> 1. For percpu stat, percpu stats are added to the return value;
> 2. For non-percpu stat, non-percpu stats will overwrite the
>    return value;
> We should keep the same semantics for both type.
>
> This patch makes percpu stats follow non-percpu's manner by
> reset the return bstats before add the percpu bstats to it.
> Also changes the caller in sch_mq.c/sch_mqprio.c to make sure
> they dump the right statistics for percpu qdisc.
>
> One more thing, the sch->q.qlen is not set with nonlock child
> qdisc in mq_dump()/mqprio_dump(), add that.
>
> Fixes: 22e0f8b9322c ("net: sched: make bstats per cpu and estimator RCU safe")
> Signed-off-by: Dust Li <dust.li@...ux.alibaba.com>
> Signed-off-by: Tony Lu <tonylu@...ux.alibaba.com>
> ---
>  net/core/gen_stats.c   |  2 ++
>  net/sched/sch_mq.c     | 34 ++++++++++++++++------------------
>  net/sched/sch_mqprio.c | 35 +++++++++++++++++------------------
>  3 files changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 1d653fbfcf52..d71af69196c9 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -120,6 +120,7 @@ __gnet_stats_copy_basic_cpu(struct gnet_stats_basic_packed *bstats,
>  {
>         int i;
>
> +       memset(bstats, 0, sizeof(*bstats));
>         for_each_possible_cpu(i) {
>                 struct gnet_stats_basic_cpu *bcpu = per_cpu_ptr(cpu, i);
>                 unsigned int start;
> @@ -288,6 +289,7 @@ __gnet_stats_copy_queue_cpu(struct gnet_stats_queue *qstats,
>  {
>         int i;
>
> +       memset(qstats, 0, sizeof(*qstats));


I think its caller is responsible to clear the stats, so you don't need to
clear them here? It looks like you do memset() twice.

Does this patch fix any bug? It looks more like a clean up to me, if so
please mark it for net-next.

Thanks.