[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK8P3a2oWT2yob76QQHDm0z7z6xcVoRDEejj7ro4heQYyWGQ3A@mail.gmail.com>
Date: Thu, 26 Mar 2020 15:31:45 +0100
From: Arnd Bergmann <arnd@...db.de>
To: Simon Chopin <s.chopin@...halink.fr>
Cc: Networking <netdev@...r.kernel.org>,
"David S . Miller" <davem@...emloft.net>,
Guillaume Nault <gnault@...hat.com>
Subject: Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats
On Thu, Mar 26, 2020 at 2:48 PM Simon Chopin <s.chopin@...halink.fr> wrote:
> Le 26/03/2020 à 11:42, Arnd Bergmann a écrit :
> > The patch looks fine from from an interface design perspective,
> > but I wonder if you could use a definition that matches the
> > structure layout and command number for PPPIOCGL2TPSTATS
> > exactly, rather than a "very similar mechanism" with a subset
> > of the fields. You would clearly have to pass down a number of
> > zero fields, but the implementation could be abstracted at a
> > higher level later.
> >
> > Arnd
>
> This sounds like a good idea, indeed. Is what follows what you had in mind ?
> I'm not too sure about keeping the chan_priv field in this form, my knowledge
> of alignment issues being relatively superficial. As I understand it, the matching
> fields in l2tp_ioc_stats should always be packed to 8 bytes as they fall on natural
> boundaries, but I might be wrong ?
>
> diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
> index a0abc68eceb5..803cbe374fb2 100644
> --- a/include/uapi/linux/ppp-ioctl.h
> +++ b/include/uapi/linux/ppp-ioctl.h
> @@ -79,14 +79,21 @@ struct pppol2tp_ioc_stats {
> __aligned_u64 rx_errors;
> };
>
> -/* For PPPIOCGPPPOESTATS */
> -struct pppoe_ioc_stats {
> +struct pppchan_ioc_stats {
> + __u8 chan_priv[8];
> __aligned_u64 tx_packets;
> __aligned_u64 tx_bytes;
> + __aligned_u64 tx_errors;
> __aligned_u64 rx_packets;
> __aligned_u64 rx_bytes;
> + __aligned_u64 rx_seq_discards;
> + __aligned_u64 rx_oos_packets;
> + __aligned_u64 rx_errors;
> };
>
> +_Static_assert(sizeof(struct pppol2tp_ioc_stats) == sizeof(struct pppchan_ioc_stats), "same size");
> +_Static_assert((size_t)&((struct pppol2tp_ioc_stats *)0)->tx_packets == (size_t)&((struct pppchan_ioc_stats *)0)->tx_packets, "same offset");
Conceptually this is what I had in mind, but implementation-wise, I'd suggest
only having a single structure definition, possibly with a #define like
#define pppoe_ioc_stats pppchan_ioc_stats
When having two struct definitions, I'd be slightly worried about
the bitfield causing implementation-defined structure layout,
so it seems better to only have one definition, even when you cannot
avoid the bitfield that maybe should not have been used.
> /*
> * Ioctl definitions.
> */
> @@ -123,7 +130,7 @@ struct pppoe_ioc_stats {
> #define PPPIOCATTCHAN _IOW('t', 56, int) /* attach to ppp channel */
> #define PPPIOCGCHAN _IOR('t', 55, int) /* get ppp channel number */
> #define PPPIOCGL2TPSTATS _IOR('t', 54, struct pppol2tp_ioc_stats)
> -#define PPPIOCGPPPOESTATS _IOR('t', 53, struct pppoe_ioc_stats)
> +#define PPPIOCGCHANSTATS _IOR('t', 54, struct pppchan_ioc_stats)
here I'd do
#define PPPIOCGCHANSTATS _IOR('t', 54, struct pppchan_ioc_stats)
#define PPPIOCGL2TPSTATS PPPIOCGCHANSTATS
Arnd
Powered by blists - more mailing lists