netdev - Re: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole

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: <CAO6EAnUPrLZzDzm6KJDaej=S4La_z01RHX2WZa3R1wTjPc09RQ@mail.gmail.com>
Date: Wed, 28 Aug 2024 11:03:09 -0400
From: Maksym Kutsevol <max@...sevol.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Breno Leitao <leitao@...ian.org>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole

Hey Jakub,
thanks for looking into this.

PS. A couple more email send mistakes and I'll go install mutt, sorry
for the noise :)

On Tue, Aug 27, 2024 at 9:59 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Mon, 26 Aug 2024 19:55:36 -0400 Maksym Kutsevol wrote:
> > > > +static ssize_t stats_show(struct config_item *item, char *buf)
> > > > +{
> > > > +     struct netconsole_target *nt = to_target(item);
> > > > +
> > > > +     return
> > > > +             nt->stats.xmit_drop_count, nt->stats.enomem_count);
> > >
> > > does configfs require value per file like sysfs or this is okay?
> >
> > Docs say (Documentation/filesystems/sysfs.txt):
> >
> > Attributes should be ASCII text files, preferably with only one value
> > per file. It is noted that it may not be efficient to contain only one
> > value per file, so it is socially acceptable to express an array of
> > values of the same type.
>
> Right, but this is for sysfs, main question is whether configfs has
> the same expectations.
Eh, my bad, thank you :)

Docs on configfs (Documentation/filesystems/configfs.rst) say approximately
the same, quote:
* Normal attributes, which similar to sysfs attributes, are small ASCII text
  files, with a maximum size of one page (PAGE_SIZE, 4096 on i386).  Preferably
  only one value per file should be used, and the same caveats from sysfs apply.
  Configfs expects write(2) to store the entire buffer at once.  When writing to
  normal configfs attributes, userspace processes should first read the entire
  file, modify the portions they wish to change, and then write the entire
  buffer back.

so based on sysfs+configfs docs it looks ok to do so. What do you think?

Regarding the overall idea of exposing stats via configfs I found this:
https://github.com/torvalds/linux/blob/master/drivers/target/iscsi/iscsi_target_stat.c#L82-L87
as an example of another place doing it, which exposes the number of
active sessions.

> > Given those are of the same type, I thought it's ok. To make it less
> > "fancy" maybe move to
> > just values separated by whitespace + a block in
> > Documentation/networking/netconsole.rst describing the format?
> > E.g. sysfs_emit(buf, "%lu %lu\n", .....) ? I really don't want to have
> > multiple files for it.
> > What do you think?
>
> Stats as an array are quite hard to read / understand
I agree with that.
I couldn't find examples of multiple values exported as stats from
configfs. Only from sysfs,
e.g. https://www.kernel.org/doc/Documentation/block/stat.txt, which
describes a whitespace
separated file with stats.

I want to lean on the opinion of someone more experienced in kernel
dev on how to proceed here.
- as is
- whitespace separated like blockdev stats
- multiple files and stop talking about it? :)