netdev - Re: [PATCH net-next 02/12] net/mlx5e: Statistics handling refactoring

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] [day] [month] [year] [list]

Date:	Mon, 25 Apr 2016 15:12:39 +0300
From:	Saeed Mahameed <saeedm@....mellanox.co.il>
To:	David Laight <David.Laight@...lab.com>
Cc:	Saeed Mahameed <saeedm@...lanox.com>,
	"David S. Miller" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Tal Alon <talal@...lanox.com>,
	Eran Ben Elisha <eranbe@...lanox.com>,
	Gal Pressman <galp@...lanox.com>
Subject: Re: [PATCH net-next 02/12] net/mlx5e: Statistics handling refactoring

On Mon, Apr 25, 2016 at 1:58 PM, David Laight <David.Laight@...lab.com> wrote:
> From: Saeed Mahameed
>> Sent: 22 April 2016 20:01
>> Redesign the statistics in the driver:
>> 1. Move counters to a separate file (en_stats.h).
>> 2. Remove unnecessary dependencies between stats and strings.
>> 3. Use counter descriptors which hold a name and offset for each counter,
>>    and will be used to decide which counters will be exposed.
>>
>> For example when adding a new software counter to ethtool, instead of:
>> 1. Add to stats struct.
>> 2. Add to strings struct in the same order.
>> 3. Change macro defining number of software counters.
>> The only thing needed is to link the new counter to a counter descriptor.
> ...
>
> This looks like a place for some pre-processor magic to define two (or more)
> items from the same line of C source.
>
> Taking one definition. Instead of:
>
>> +static const struct counter_desc vport_stats_desc[] = {
>> +     { "rx_error_packets", VPORT_COUNTER_OFF(received_errors.packets) },
>> +     { "rx_error_bytes", VPORT_COUNTER_OFF(received_errors.octets) },
>> +     { "tx_error_packets", VPORT_COUNTER_OFF(transmit_errors.packets) },
>> +     { "tx_error_bytes", VPORT_COUNTER_OFF(transmit_errors.octets) },
>
> Use the following #define to generate both the array of names and the structure.
> #define VPORT_STATS_DEFN(RX, TX) \
>         RX( "rx_error_packets", packets) \
>         RX( "rx_error_bytes", octets) \
>         TX( "tx_error_packets", packets) \
>         TX( "tx_error_bytes", octets) \
>         ...
>
> You can then expand it two (or more) ways:
>
> #define STATS_NAMES(name, field) name,
> static const char *const vport_stats_names[] = { VPORT_STATS_DEFN(STATS_NAMES, STATS_NAMES) };
>

Ok here you accumulated all the RX,TX names ! and now it looks like we
are back to square one.

> #define STAT_FIELD(name, field) uint64_t field;
> #define STAT_NONE(name, field)
> struct received_errors {
>         VPORT_STATS_DEFN(STAT_FIELD, STAT_NONE)
> };
> struct transmit_errors errors {
>         VPORT_STATS_DEFN(STAT_NONE, STAT_FIELD)
> };
>

And now my head is spinning ! how do you get the field offset ?
VPORT_COUNTER_OFF

> Adding an extra stat then just requires you add an extra line to VPORT_STATS_DEFN().
>

IMHO this is too much magic ! also in the current implementation to
add extra stat you just add extra line:
in vport_stats_desc[] = {
...
 { "new_stat", VPORT_COUNTER_OFF(new_stat_field) },
}

Why would you complicate it ? cool trick though, but i wouldn't take
it to my driver.

Oh, my head is still spinning :) ..

Thanks,
Saeed.