[PATCH net-next] pppoe: new ioctl to extract per-channel stats

[PATCH net-next] pppoe: new ioctl to extract per-channel stats

[Simon Chopin]

The PPP subsystem uses the abstractions of channels and units, the
latter being an aggregate of the former, exported to userspace as a
single network interface.  As such, it keeps traffic statistics at the
unit level, but there are no statistics on the individual channels,
partly because most PPP units only have one channel.

However, it is sometimes useful to have statistics at the channel level,
for instance to monitor multilink PPP connections. Such statistics
already exist for PPPoL2TP via the PPPIOCGL2TPSTATS ioctl, this patch
introduces a very similar mechanism for PPPoE via a new
PPPIOCGPPPOESTATS ioctl.

The contents of this patch were greatly inspired by the L2TP
implementation, many thanks to its authors.

Signed-off-by: Simon Chopin <s.chopin@alphalink.fr>
---
 drivers/net/ppp/pppoe.c        | 45 ++++++++++++++++++++++++++++++++++
 include/uapi/linux/ppp-ioctl.h |  9 +++++++
 2 files changed, 54 insertions(+)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index d760a36db28c..6a3d98d720d5 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -72,6 +72,7 @@
 #include <linux/file.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/atomic.h>
 
 #include <linux/nsproxy.h>
 #include <net/net_namespace.h>
@@ -104,6 +105,13 @@ struct pppoe_net {
 	rwlock_t hash_lock;
 };
 
+struct pppoe_stats {
+	atomic_long_t	tx_packets;
+	atomic_long_t	tx_bytes;
+	atomic_long_t	rx_packets;
+	atomic_long_t	rx_bytes;
+};
+
 /*
  * PPPoE could be in the following stages:
  * 1) Discovery stage (to obtain remote MAC and Session ID)
@@ -366,6 +374,8 @@ static int pppoe_rcv_core(struct sock *sk, struct sk_buff *skb)
 {
 	struct pppox_sock *po = pppox_sk(sk);
 	struct pppox_sock *relay_po;
+	struct pppoe_stats *stats;
+	size_t len = skb->len;
 
 	/* Backlog receive. Semantics of backlog rcv preclude any code from
 	 * executing in lock_sock()/release_sock() bounds; meaning sk->sk_state
@@ -395,6 +405,10 @@ static int pppoe_rcv_core(struct sock *sk, struct sk_buff *skb)
 			goto abort_kfree;
 	}
 
+	stats = sk_pppox(po)->sk_user_data;
+	atomic_long_inc(&stats->rx_packets);
+	atomic_long_add(len, &stats->rx_bytes);
+
 	return NET_RX_SUCCESS;
 
 abort_put:
@@ -549,6 +563,8 @@ static int pppoe_create(struct net *net, struct socket *sock, int kern)
 	sk->sk_family		= PF_PPPOX;
 	sk->sk_protocol		= PX_PROTO_OE;
 
+	sk->sk_user_data = kzalloc(sizeof(struct pppoe_stats), GFP_KERNEL);
+
 	INIT_WORK(&pppox_sk(sk)->proto.pppoe.padt_work,
 		  pppoe_unbind_sock_work);
 
@@ -593,6 +609,8 @@ static int pppoe_release(struct socket *sock)
 	delete_item(pn, po->pppoe_pa.sid, po->pppoe_pa.remote,
 		    po->pppoe_ifindex);
 
+	kfree(sk->sk_user_data);
+	sk->sk_user_data = NULL;
 	sock_orphan(sk);
 	sock->sk = NULL;
 
@@ -730,11 +748,23 @@ static int pppoe_getname(struct socket *sock, struct sockaddr *uaddr,
 	return len;
 }
 
+static void pppoe_copy_stats(struct pppoe_ioc_stats *dest,
+			     const struct pppoe_stats *stats)
+{
+	memset(dest, 0, sizeof(*dest));
+
+	dest->tx_packets = atomic_long_read(&stats->tx_packets);
+	dest->tx_bytes = atomic_long_read(&stats->tx_bytes);
+	dest->rx_packets = atomic_long_read(&stats->rx_packets);
+	dest->rx_bytes = atomic_long_read(&stats->rx_bytes);
+}
+
 static int pppoe_ioctl(struct socket *sock, unsigned int cmd,
 		unsigned long arg)
 {
 	struct sock *sk = sock->sk;
 	struct pppox_sock *po = pppox_sk(sk);
+	struct pppoe_ioc_stats stats;
 	int val;
 	int err;
 
@@ -823,6 +853,18 @@ static int pppoe_ioctl(struct socket *sock, unsigned int cmd,
 		err = 0;
 		break;
 
+	case PPPIOCGPPPOESTATS:
+		err = -EBUSY;
+		/* Check that the stats struct hasn't been freed yet */
+		if (sk->sk_state & PPPOX_DEAD)
+			break;
+
+		pppoe_copy_stats(&stats, sk->sk_user_data);
+		err = 0;
+		if (copy_to_user((void __user *)arg, &stats, sizeof(stats)))
+			err = -EFAULT;
+		break;
+
 	default:
 		err = -ENOTTY;
 	}
@@ -910,6 +952,7 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb)
 {
 	struct pppox_sock *po = pppox_sk(sk);
 	struct net_device *dev = po->pppoe_dev;
+	struct pppoe_stats *stats = sk->sk_user_data;
 	struct pppoe_hdr *ph;
 	int data_len = skb->len;
 
@@ -950,6 +993,8 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb)
 			po->pppoe_pa.remote, NULL, data_len);
 
 	dev_queue_xmit(skb);
+	atomic_long_inc(&stats->tx_packets);
+	atomic_long_add(data_len, &stats->tx_bytes);
 	return 1;
 
 abort:
diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
index 7bd2a5a75348..a0abc68eceb5 100644
--- a/include/uapi/linux/ppp-ioctl.h
+++ b/include/uapi/linux/ppp-ioctl.h
@@ -79,6 +79,14 @@ struct pppol2tp_ioc_stats {
 	__aligned_u64	rx_errors;
 };
 
+/* For PPPIOCGPPPOESTATS */
+struct pppoe_ioc_stats {
+	__aligned_u64	tx_packets;
+	__aligned_u64	tx_bytes;
+	__aligned_u64	rx_packets;
+	__aligned_u64	rx_bytes;
+};
+
 /*
  * Ioctl definitions.
  */
@@ -115,6 +123,7 @@ struct pppol2tp_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 SIOCGPPPSTATS   (SIOCDEVPRIVATE + 0)
 #define SIOCGPPPVER     (SIOCDEVPRIVATE + 1)	/* NEVER change this!! */

base-commit: 16fbf79b0f83bc752cee8589279f1ebfe57b3b6e
-- 
2.25.1

Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

[Arnd Bergmann]

On Thu, Mar 26, 2020 at 11:32 AM Simon Chopin <s.chopin@alphalink.fr> wrote:
>
> The PPP subsystem uses the abstractions of channels and units, the
> latter being an aggregate of the former, exported to userspace as a
> single network interface.  As such, it keeps traffic statistics at the
> unit level, but there are no statistics on the individual channels,
> partly because most PPP units only have one channel.
>
> However, it is sometimes useful to have statistics at the channel level,
> for instance to monitor multilink PPP connections. Such statistics
> already exist for PPPoL2TP via the PPPIOCGL2TPSTATS ioctl, this patch
> introduces a very similar mechanism for PPPoE via a new
> PPPIOCGPPPOESTATS ioctl.
>
> The contents of this patch were greatly inspired by the L2TP
> implementation, many thanks to its authors.

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

Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

[Simon Chopin]

(Dropping Michal Ostrowski from the CC list as the address bounces for me)

Le 26/03/2020 à 11:42, Arnd Bergmann a écrit :
> On Thu, Mar 26, 2020 at 11:32 AM Simon Chopin <s.chopin@alphalink.fr> wrote:
>> The PPP subsystem uses the abstractions of channels and units, the
>> latter being an aggregate of the former, exported to userspace as a
>> single network interface.  As such, it keeps traffic statistics at the
>> unit level, but there are no statistics on the individual channels,
>> partly because most PPP units only have one channel.
>>
>> However, it is sometimes useful to have statistics at the channel level,
>> for instance to monitor multilink PPP connections. Such statistics
>> already exist for PPPoL2TP via the PPPIOCGL2TPSTATS ioctl, this patch
>> introduces a very similar mechanism for PPPoE via a new
>> PPPIOCGPPPOESTATS ioctl.
>>
>> The contents of this patch were greatly inspired by the L2TP
>> implementation, many thanks to its authors.
> 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 ?

  Simon


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");
+
 /*
  * 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)

 #define SIOCGPPPSTATS   (SIOCDEVPRIVATE + 0)
 #define SIOCGPPPVER     (SIOCDEVPRIVATE + 1)   /* NEVER change this!! */

Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

[Arnd Bergmann]

On Thu, Mar 26, 2020 at 2:48 PM Simon Chopin <s.chopin@alphalink.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

Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

[Simon Chopin]

Hello Arnd,

Le 26/03/2020 à 15:31, Arnd Bergmann a écrit :
> On Thu, Mar 26, 2020 at 2:48 PM Simon Chopin <s.chopin@alphalink.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
I'm assuming that'd be #define pppol2tp_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

Thank you for your feedback. I'm probably going to implement a more
generic version at the generic PPP channel instead, though, as,
as noted by Guillaume, those statistics are not for the PPP channel
but for the layer underneath.

However, I'd like to be sure I understand your proposal here :
we'd use a generic pppchan_ioc_stats struct that would be identical
to the current pppol2tp_ioc_stats, including the 3 L2TP-specific fields,
so that we'd retain ABI and API compatibility, and we would simply
#define the current API to the new one?

Cheers,
Simon

Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

[Arnd Bergmann]

On Tue, Mar 31, 2020 at 12:03 PM Simon Chopin <s.chopin@alphalink.fr> wrote:
> Le 26/03/2020 à 15:31, Arnd Bergmann a écrit :
> > On Thu, Mar 26, 2020 at 2:48 PM Simon Chopin <s.chopin@alphalink.fr> wrote:
> >> +_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
> I'm assuming that'd be #define pppol2tp_stats pppchan_ioc_stats ?

Right.

> > #define PPPIOCGCHANSTATS _IOR('t', 54, struct pppchan_ioc_stats)
> > #define PPPIOCGL2TPSTATS PPPIOCGCHANSTATS
>
> Thank you for your feedback. I'm probably going to implement a more
> generic version at the generic PPP channel instead, though, as,
> as noted by Guillaume, those statistics are not for the PPP channel
> but for the layer underneath.
>
> However, I'd like to be sure I understand your proposal here :
> we'd use a generic pppchan_ioc_stats struct that would be identical
> to the current pppol2tp_ioc_stats, including the 3 L2TP-specific fields,
> so that we'd retain ABI and API compatibility, and we would simply
> #define the current API to the new one?

Yes, that's the idea.

      Arnd

Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

[Guillaume Nault]

On Thu, Mar 26, 2020 at 11:32:30AM +0100, Simon Chopin wrote:
> The PPP subsystem uses the abstractions of channels and units, the
> latter being an aggregate of the former, exported to userspace as a
> single network interface.  As such, it keeps traffic statistics at the
> unit level, but there are no statistics on the individual channels,
> partly because most PPP units only have one channel.
> 
> However, it is sometimes useful to have statistics at the channel level,
> for instance to monitor multilink PPP connections. Such statistics
> already exist for PPPoL2TP via the PPPIOCGL2TPSTATS ioctl, this patch
> introduces a very similar mechanism for PPPoE via a new
> PPPIOCGPPPOESTATS ioctl.
> 
I'd rather recomment _not_ using multilink PPP over PPPoE (or L2TP, or
any form of overlay network). But apart from that, I find the
description misleading. PPPoE is not a PPP channel, it _transports_ a
channel. PPPoE might not even be associated with a channel at all,
like in the PPPOX_RELAY case. In short PPPoE stats aren't channel's
stats. If the objective it to get channels stats, then this needs to be
implemented in ppp_generic.c. If what you really want is PPPoE stats,
then see my comments below.

> @@ -395,6 +405,10 @@ static int pppoe_rcv_core(struct sock *sk, struct sk_buff *skb)
>  			goto abort_kfree;
>  	}
>  
> +	stats = sk_pppox(po)->sk_user_data;
> +	atomic_long_inc(&stats->rx_packets);
> +	atomic_long_add(len, &stats->rx_bytes);
> +
>  	return NET_RX_SUCCESS;
>  
You probably need to add counter(s) for the error paths too.

> @@ -549,6 +563,8 @@ static int pppoe_create(struct net *net, struct socket *sock, int kern)
>  	sk->sk_family		= PF_PPPOX;
>  	sk->sk_protocol		= PX_PROTO_OE;
>  
> +	sk->sk_user_data = kzalloc(sizeof(struct pppoe_stats), GFP_KERNEL);
> +
Missing error check.

But please don't use ->sk_user_data for that. We have enough problems
with this pointer, let's not add users that don't actually need it.
See https://lore.kernel.org/netdev/20180117.142538.1972806008716856078.davem@davemloft.net/
for some details.
You can store the counters inside the socket instead.

> @@ -950,6 +993,8 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb)
>  			po->pppoe_pa.remote, NULL, data_len);
>  
>  	dev_queue_xmit(skb);
> +	atomic_long_inc(&stats->tx_packets);
> +	atomic_long_add(data_len, &stats->tx_bytes);
>  	return 1;
>  
Again, you probably need to count errors too.

Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

[Simon Chopin]

Hello Guillaume,

Le 26/03/2020 à 15:38, Guillaume Nault a écrit :
> On Thu, Mar 26, 2020 at 11:32:30AM +0100, Simon Chopin wrote:
>> The PPP subsystem uses the abstractions of channels and units, the
>> latter being an aggregate of the former, exported to userspace as a
>> single network interface.  As such, it keeps traffic statistics at the
>> unit level, but there are no statistics on the individual channels,
>> partly because most PPP units only have one channel.
>>
>> However, it is sometimes useful to have statistics at the channel level,
>> for instance to monitor multilink PPP connections. Such statistics
>> already exist for PPPoL2TP via the PPPIOCGL2TPSTATS ioctl, this patch
>> introduces a very similar mechanism for PPPoE via a new
>> PPPIOCGPPPOESTATS ioctl.
>>
> I'd rather recomment _not_ using multilink PPP over PPPoE (or L2TP, or
> any form of overlay network). But apart from that, I find the
> description misleading. PPPoE is not a PPP channel, it _transports_ a
> channel. PPPoE might not even be associated with a channel at all,
> like in the PPPOX_RELAY case. In short PPPoE stats aren't channel's
> stats. If the objective it to get channels stats, then this needs to be
> implemented in ppp_generic.c. If what you really want is PPPoE stats,
> then see my comments below.

Thank you for your feedback
I indeed want some statistics over PPP channels, notably over L2TP and
PPPoE. Since a mechanism already existed for the former, I thought
it simpler to implement the same for the latter, but your point makes sense:
those subsystems operate below the PPP layer, with extra control packets
and header overhead.

<snip>

>> @@ -549,6 +563,8 @@ static int pppoe_create(struct net *net, struct socket *sock, int kern)
>>  	sk->sk_family		= PF_PPPOX;
>>  	sk->sk_protocol		= PX_PROTO_OE;
>>  
>> +	sk->sk_user_data = kzalloc(sizeof(struct pppoe_stats), GFP_KERNEL);
>> +
> Missing error check.
> 
> But please don't use ->sk_user_data for that. We have enough problems
> with this pointer, let's not add users that don't actually need it.
> See https://lore.kernel.org/netdev/20180117.142538.1972806008716856078.davem@davemloft.net/
> for some details.
> You can store the counters inside the socket instead.

Thank you for the pointers. I'll pay attention to error paths in any further
version, and in any case will drop the sk_user_data use.

Would it be allright to post new patches as RFC before net-next opens in
order to get further feedback, or is that frowned upon ?

Cheers,
Simon

Re: [PATCH net-next] pppoe: new ioctl to extract per-channel stats

[Guillaume Nault]

On Tue, Mar 31, 2020 at 11:27:23AM +0200, Simon Chopin wrote:
> Hello Guillaume,
> 
> Le 26/03/2020 à 15:38, Guillaume Nault a écrit :
> > On Thu, Mar 26, 2020 at 11:32:30AM +0100, Simon Chopin wrote:
> >> The PPP subsystem uses the abstractions of channels and units, the
> >> latter being an aggregate of the former, exported to userspace as a
> >> single network interface.  As such, it keeps traffic statistics at the
> >> unit level, but there are no statistics on the individual channels,
> >> partly because most PPP units only have one channel.
> >>
> >> However, it is sometimes useful to have statistics at the channel level,
> >> for instance to monitor multilink PPP connections. Such statistics
> >> already exist for PPPoL2TP via the PPPIOCGL2TPSTATS ioctl, this patch
> >> introduces a very similar mechanism for PPPoE via a new
> >> PPPIOCGPPPOESTATS ioctl.
> >>
> > I'd rather recomment _not_ using multilink PPP over PPPoE (or L2TP, or
> > any form of overlay network). But apart from that, I find the
> > description misleading. PPPoE is not a PPP channel, it _transports_ a
> > channel. PPPoE might not even be associated with a channel at all,
> > like in the PPPOX_RELAY case. In short PPPoE stats aren't channel's
> > stats. If the objective it to get channels stats, then this needs to be
> > implemented in ppp_generic.c. If what you really want is PPPoE stats,
> > then see my comments below.
> 
> Thank you for your feedback
> I indeed want some statistics over PPP channels, notably over L2TP and
> PPPoE. Since a mechanism already existed for the former, I thought
> it simpler to implement the same for the latter, but your point makes sense:
> those subsystems operate below the PPP layer, with extra control packets
> and header overhead.
> 
Then I guess getting statistics of the PPP channel is more appropriate.
It might be possible to move the ppp_link_stats structure from
struct ppp to struct ppp_file, so that it could be used for channels
and for units. If necessary, the structure can probably be extended to
record more statistics.

> >> @@ -549,6 +563,8 @@ static int pppoe_create(struct net *net, struct socket *sock, int kern)
> >>  	sk->sk_family		= PF_PPPOX;
> >>  	sk->sk_protocol		= PX_PROTO_OE;
> >>  
> >> +	sk->sk_user_data = kzalloc(sizeof(struct pppoe_stats), GFP_KERNEL);
> >> +
> > Missing error check.
> > 
> > But please don't use ->sk_user_data for that. We have enough problems
> > with this pointer, let's not add users that don't actually need it.
> > See https://lore.kernel.org/netdev/20180117.142538.1972806008716856078.davem@davemloft.net/
> > for some details.
> > You can store the counters inside the socket instead.
> 
> Thank you for the pointers. I'll pay attention to error paths in any further
> version, and in any case will drop the sk_user_data use.
> 
> Would it be allright to post new patches as RFC before net-next opens in
> order to get further feedback, or is that frowned upon ?
> 
It should be fine, as long as it's clearly indicated in the subject.