MIME-Version: 1.0
References: <20230311151756.83302-1-kerneljasonxing@gmail.com>
 <ZA4huzYKK/tdT3Ep@corigine.com> <CAL+tcoDi5fVWjyTX6wjJGKrszqL6JWkEgDBajhZchYSW7kyhGQ@mail.gmail.com>
 <20230312192847.0d58155e@hermes.local>
In-Reply-To: <20230312192847.0d58155e@hermes.local>
From:   Jason Xing <kerneljasonxing@gmail.com>
Date:   Mon, 13 Mar 2023 17:03:07 +0800
Message-ID: <CAL+tcoAqAFxLJyXCSX_=2zPLWuEZ_WhbA04FC1UG+KisE_UNYA@mail.gmail.com>
Subject: Re: [PATCH net-next] net-sysfs: display two backlog queue len separately
To:     Stephen Hemminger <stephen@networkplumber.org>
Cc:     Simon Horman <simon.horman@corigine.com>, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jason Xing <kernelxing@tencent.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Mar 13, 2023 at 10:28=E2=80=AFAM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Mon, 13 Mar 2023 09:55:37 +0800
> Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > On Mon, Mar 13, 2023 at 3:02=E2=80=AFAM Simon Horman <simon.horman@cori=
gine.com> wrote:
> > >
> > > On Sat, Mar 11, 2023 at 11:17:56PM +0800, Jason Xing wrote:
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > Sometimes we need to know which one of backlog queue can be exactly
> > > > long enough to cause some latency when debugging this part is neede=
d.
> > > > Thus, we can then separate the display of both.
> > > >
> > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > ---
> > > >  net/core/net-procfs.c | 17 ++++++++++++-----
> > > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
> > > > index 1ec23bf8b05c..97a304e1957a 100644
> > > > --- a/net/core/net-procfs.c
> > > > +++ b/net/core/net-procfs.c
> > > > @@ -115,10 +115,14 @@ static int dev_seq_show(struct seq_file *seq,=
 void *v)
> > > >       return 0;
> > > >  }
> > > >
> > > > -static u32 softnet_backlog_len(struct softnet_data *sd)
> > > > +static u32 softnet_input_pkt_queue_len(struct softnet_data *sd)
> > > >  {
> > > > -     return skb_queue_len_lockless(&sd->input_pkt_queue) +
> > > > -            skb_queue_len_lockless(&sd->process_queue);
> > > > +     return skb_queue_len_lockless(&sd->input_pkt_queue);
> > > > +}
> > > > +
> > > > +static u32 softnet_process_queue_len(struct softnet_data *sd)
> > > > +{
> > > > +     return skb_queue_len_lockless(&sd->process_queue);
> > > >  }
> > > >
> > > >  static struct softnet_data *softnet_get_online(loff_t *pos)
> > > > @@ -169,12 +173,15 @@ static int softnet_seq_show(struct seq_file *=
seq, void *v)
> > > >        * mapping the data a specific CPU
> > > >        */
> > > >       seq_printf(seq,
> > > > -                "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x=
 %08x %08x %08x\n",
> > > > +                "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x=
 %08x %08x %08x "
> > > > +                "%08x %08x\n",
> > > >                  sd->processed, sd->dropped, sd->time_squeeze, 0,
> > > >                  0, 0, 0, 0, /* was fastroute */
> > > >                  0,   /* was cpu_collision */
> > > >                  sd->received_rps, flow_limit_count,
> > > > -                softnet_backlog_len(sd), (int)seq->index);
> > > > +                0,   /* was len of two backlog queues */
> > > > +                (int)seq->index,
> > >
> > > nit: I think you could avoid this cast by using %llx as the format sp=
ecifier.
> >
> > I'm not sure if I should change this format since the above line is
> > introduced in commit 7d58e6555870d ('net-sysfs: add backlog len and
> > CPU id to softnet data').
> > The seq->index here manifests which cpu it uses, so it can be
> > displayed in 'int' format. Meanwhile, using %8x to output is much
> > cleaner if the user executes 'cat /proc/net/softnet_stat'.
> >
> > What do you think about this?
> >
> > Thanks,
> > Jason
>
> I consider sofnet_data a legacy API (ie don't change).
[...]
> Why not add to real sysfs for network device with the one value per file?

Well, I'm wondering if the way you suggested is probably not proper
because this structure is per cpu which means that we have 'num_cpus'
* 'how many members we should print' files. It's too many, I think.

/proc/net/softnet_stat is still a good choice :) I need more advice on this=
.

Thanks,
Jason