[PATCH net-next 0/3] ppp: improve receive path performance

[PATCH net-next 0/3] ppp: improve receive path performance

[Qingfang Deng]

This patch series improves the performance of the PPPoE receive paths.

Patch 1 converts the ppp->rlock from a spinlock to a read-write lock,
allowing concurrent receive-side processing when no state is being
modified.

Patch 2 optimizes PPPoE receive performance by bypassing sk_receive_skb()
when the socket is in the PPPOX_BOUND state, avoiding unnecessary socket
locking and overhead.

Patch 3 synchronizes all updates to net_device->stats using
DEV_STATS_INC() to prevent data races now that the receive path may run on
multiple CPUs.

Qingfang Deng (3):
  ppp: convert rlock to rwlock to improve RX concurrency
  pppoe: call ppp_input directly when PPPOX_BOUND
  ppp: synchronize netstats updates

 drivers/net/ppp/ppp_generic.c | 32 +++++++++++++++++---------------
 drivers/net/ppp/pppoe.c       | 10 +++++++++-
 2 files changed, 26 insertions(+), 16 deletions(-)

-- 
2.43.0

[PATCH net-next 1/3] ppp: convert rlock to rwlock to improve RX concurrency

[Qingfang Deng]

The rlock spinlock in struct ppp protects the receive path, but it is
frequently used in a read-mostly manner. Converting it to rwlock_t
allows multiple CPUs to concurrently enter the receive path (e.g.,
ppp_do_recv()), improving RX performance.

Write locking is preserved for code paths that mutate state.

Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
 drivers/net/ppp/ppp_generic.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 4cf9d1822a83..f0f8a75e3aef 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -118,7 +118,7 @@ struct ppp {
 	struct file	*owner;		/* file that owns this unit 48 */
 	struct list_head channels;	/* list of attached channels 4c */
 	int		n_channels;	/* how many channels are attached 54 */
-	spinlock_t	rlock;		/* lock for receive side 58 */
+	rwlock_t	rlock;		/* lock for receive side 58 */
 	spinlock_t	wlock;		/* lock for transmit side 5c */
 	int __percpu	*xmit_recursion; /* xmit recursion detect */
 	int		mru;		/* max receive unit 60 */
@@ -371,12 +371,14 @@ static const int npindex_to_ethertype[NUM_NP] = {
  */
 #define ppp_xmit_lock(ppp)	spin_lock_bh(&(ppp)->wlock)
 #define ppp_xmit_unlock(ppp)	spin_unlock_bh(&(ppp)->wlock)
-#define ppp_recv_lock(ppp)	spin_lock_bh(&(ppp)->rlock)
-#define ppp_recv_unlock(ppp)	spin_unlock_bh(&(ppp)->rlock)
+#define ppp_recv_lock(ppp)	write_lock_bh(&(ppp)->rlock)
+#define ppp_recv_unlock(ppp)	write_unlock_bh(&(ppp)->rlock)
 #define ppp_lock(ppp)		do { ppp_xmit_lock(ppp); \
 				     ppp_recv_lock(ppp); } while (0)
 #define ppp_unlock(ppp)		do { ppp_recv_unlock(ppp); \
 				     ppp_xmit_unlock(ppp); } while (0)
+#define ppp_recv_read_lock(ppp)		read_lock_bh(&(ppp)->rlock)
+#define ppp_recv_read_unlock(ppp)	read_unlock_bh(&(ppp)->rlock)
 
 /*
  * /dev/ppp device routines.
@@ -1246,7 +1248,7 @@ static int ppp_dev_configure(struct net *src_net, struct net_device *dev,
 	for (indx = 0; indx < NUM_NP; ++indx)
 		ppp->npmode[indx] = NPMODE_PASS;
 	INIT_LIST_HEAD(&ppp->channels);
-	spin_lock_init(&ppp->rlock);
+	rwlock_init(&ppp->rlock);
 	spin_lock_init(&ppp->wlock);
 
 	ppp->xmit_recursion = alloc_percpu(int);
@@ -2193,12 +2195,12 @@ struct ppp_mp_skb_parm {
 static inline void
 ppp_do_recv(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 {
-	ppp_recv_lock(ppp);
+	ppp_recv_read_lock(ppp);
 	if (!ppp->closing)
 		ppp_receive_frame(ppp, skb, pch);
 	else
 		kfree_skb(skb);
-	ppp_recv_unlock(ppp);
+	ppp_recv_read_unlock(ppp);
 }
 
 /**
-- 
2.43.0

Re: [PATCH net-next 1/3] ppp: convert rlock to rwlock to improve RX concurrency

[Andrew Lunn]

>  #define ppp_xmit_lock(ppp)	spin_lock_bh(&(ppp)->wlock)
>  #define ppp_xmit_unlock(ppp)	spin_unlock_bh(&(ppp)->wlock)
> -#define ppp_recv_lock(ppp)	spin_lock_bh(&(ppp)->rlock)
> -#define ppp_recv_unlock(ppp)	spin_unlock_bh(&(ppp)->rlock)
> +#define ppp_recv_lock(ppp)	write_lock_bh(&(ppp)->rlock)
> +#define ppp_recv_unlock(ppp)	write_unlock_bh(&(ppp)->rlock)
>  #define ppp_lock(ppp)		do { ppp_xmit_lock(ppp); \
>  				     ppp_recv_lock(ppp); } while (0)
>  #define ppp_unlock(ppp)		do { ppp_recv_unlock(ppp); \
>  				     ppp_xmit_unlock(ppp); } while (0)
> +#define ppp_recv_read_lock(ppp)		read_lock_bh(&(ppp)->rlock)
> +#define ppp_recv_read_unlock(ppp)	read_unlock_bh(&(ppp)->rlock)

Given the _read_ in ppp_recv_read_lock(), maybe ppp_recv_lock() should
be ppp_recv_write_lock()? Makes it symmetrical, and clearer there is a
read/write lock.

	Andrew

Re: [PATCH net-next 1/3] ppp: convert rlock to rwlock to improve RX concurrency

[Guillaume Nault]

On Wed, Jun 25, 2025 at 11:40:18AM +0800, Qingfang Deng wrote:
> The rlock spinlock in struct ppp protects the receive path, but it is
> frequently used in a read-mostly manner. Converting it to rwlock_t
> allows multiple CPUs to concurrently enter the receive path (e.g.,
> ppp_do_recv()), improving RX performance.
> 
> Write locking is preserved for code paths that mutate state.
> 
> Signed-off-by: Qingfang Deng <dqfext@gmail.com>
> ---
>  drivers/net/ppp/ppp_generic.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 4cf9d1822a83..f0f8a75e3aef 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -118,7 +118,7 @@ struct ppp {
>  	struct file	*owner;		/* file that owns this unit 48 */
>  	struct list_head channels;	/* list of attached channels 4c */
>  	int		n_channels;	/* how many channels are attached 54 */
> -	spinlock_t	rlock;		/* lock for receive side 58 */
> +	rwlock_t	rlock;		/* lock for receive side 58 */
>  	spinlock_t	wlock;		/* lock for transmit side 5c */
>  	int __percpu	*xmit_recursion; /* xmit recursion detect */
>  	int		mru;		/* max receive unit 60 */
> @@ -371,12 +371,14 @@ static const int npindex_to_ethertype[NUM_NP] = {
>   */
>  #define ppp_xmit_lock(ppp)	spin_lock_bh(&(ppp)->wlock)
>  #define ppp_xmit_unlock(ppp)	spin_unlock_bh(&(ppp)->wlock)
> -#define ppp_recv_lock(ppp)	spin_lock_bh(&(ppp)->rlock)
> -#define ppp_recv_unlock(ppp)	spin_unlock_bh(&(ppp)->rlock)
> +#define ppp_recv_lock(ppp)	write_lock_bh(&(ppp)->rlock)
> +#define ppp_recv_unlock(ppp)	write_unlock_bh(&(ppp)->rlock)
>  #define ppp_lock(ppp)		do { ppp_xmit_lock(ppp); \
>  				     ppp_recv_lock(ppp); } while (0)
>  #define ppp_unlock(ppp)		do { ppp_recv_unlock(ppp); \
>  				     ppp_xmit_unlock(ppp); } while (0)
> +#define ppp_recv_read_lock(ppp)		read_lock_bh(&(ppp)->rlock)
> +#define ppp_recv_read_unlock(ppp)	read_unlock_bh(&(ppp)->rlock)
>  
>  /*
>   * /dev/ppp device routines.
> @@ -1246,7 +1248,7 @@ static int ppp_dev_configure(struct net *src_net, struct net_device *dev,
>  	for (indx = 0; indx < NUM_NP; ++indx)
>  		ppp->npmode[indx] = NPMODE_PASS;
>  	INIT_LIST_HEAD(&ppp->channels);
> -	spin_lock_init(&ppp->rlock);
> +	rwlock_init(&ppp->rlock);
>  	spin_lock_init(&ppp->wlock);
>  
>  	ppp->xmit_recursion = alloc_percpu(int);
> @@ -2193,12 +2195,12 @@ struct ppp_mp_skb_parm {
>  static inline void
>  ppp_do_recv(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
>  {
> -	ppp_recv_lock(ppp);
> +	ppp_recv_read_lock(ppp);
>  	if (!ppp->closing)
>  		ppp_receive_frame(ppp, skb, pch);

That doesn't look right. Several PPP Rx features are stateful
(multilink, compression, etc.) and the current implementations
currently don't take any precaution when updating the shared states.

For example, see how bsd_decompress() (in bsd_comp.c) updates db->*
fields all over the place. This db variable comes from ppp->rc_state,
which is passed as parameter of the ppp->rcomp->decompress() call in
ppp_decompress_frame().

I think a lot of work would be needed before we could allow
ppp_do_recv() to run concurrently on the same struct ppp.

>  	else
>  		kfree_skb(skb);
> -	ppp_recv_unlock(ppp);
> +	ppp_recv_read_unlock(ppp);
>  }
>  
>  /**
> -- 
> 2.43.0
> 

Re: [PATCH net-next 1/3] ppp: convert rlock to rwlock to improve RX concurrency

[Qingfang Deng]

On Fri, Jun 27, 2025 at 12:23 AM Guillaume Nault <gnault@redhat.com> wrote:
> That doesn't look right. Several PPP Rx features are stateful
> (multilink, compression, etc.) and the current implementations
> currently don't take any precaution when updating the shared states.
>
> For example, see how bsd_decompress() (in bsd_comp.c) updates db->*
> fields all over the place. This db variable comes from ppp->rc_state,
> which is passed as parameter of the ppp->rcomp->decompress() call in
> ppp_decompress_frame().
>
> I think a lot of work would be needed before we could allow
> ppp_do_recv() to run concurrently on the same struct ppp.

Right. I think we can grab a write lock where it updates struct ppp.

Re: [PATCH net-next 1/3] ppp: convert rlock to rwlock to improve RX concurrency

[Eric Dumazet]

On Thu, Jun 26, 2025 at 9:00 PM Qingfang Deng <dqfext@gmail.com> wrote:
>
> On Fri, Jun 27, 2025 at 12:23 AM Guillaume Nault <gnault@redhat.com> wrote:
> > That doesn't look right. Several PPP Rx features are stateful
> > (multilink, compression, etc.) and the current implementations
> > currently don't take any precaution when updating the shared states.
> >
> > For example, see how bsd_decompress() (in bsd_comp.c) updates db->*
> > fields all over the place. This db variable comes from ppp->rc_state,
> > which is passed as parameter of the ppp->rcomp->decompress() call in
> > ppp_decompress_frame().
> >
> > I think a lot of work would be needed before we could allow
> > ppp_do_recv() to run concurrently on the same struct ppp.
>
> Right. I think we can grab a write lock where it updates struct ppp.

tldr: network maintainers do not want rwlock back.

If you really care about concurrency, do not use rwlock, because it is
more expensive than a spinlock and very problematic.

Instead use RCU for readers, and spinlock for the parts needing exclusion.

Adding rwlock in network fast path is almost always a very bad choice.

Take a look at commit 0daf07e527095e6 for gory details.

Re: [PATCH net-next 1/3] ppp: convert rlock to rwlock to improve RX concurrency

[Qingfang Deng]

On Fri, Jun 27, 2025 at 2:50 PM Eric Dumazet <edumazet@google.com> wrote:
> tldr: network maintainers do not want rwlock back.
>
> If you really care about concurrency, do not use rwlock, because it is
> more expensive than a spinlock and very problematic.
>
> Instead use RCU for readers, and spinlock for the parts needing exclusion.
>
> Adding rwlock in network fast path is almost always a very bad choice.
>
> Take a look at commit 0daf07e527095e6 for gory details.

Thanks for the info, I'll see how to refactor it using RCU instead.

[PATCH net-next 2/3] pppoe: bypass sk_receive_skb for PPPOX_BOUND sockets

[Qingfang Deng]

When a PPPoE session socket is in PPPOX_BOUND state, the RX path is
fully synchronous and does not require socket receive queueing or
callbacks. Avoid calling sk_receive_skb(), which acquires the socket
lock and adds overhead.

Call ppp_input() directly to reduce lock contention and improve
performance on RX path.

Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
 drivers/net/ppp/pppoe.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 410effa42ade..ba30d7afe9ff 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -413,6 +413,7 @@ static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
 	struct pppoe_hdr *ph;
 	struct pppox_sock *po;
 	struct pppoe_net *pn;
+	struct sock *sk;
 	int len;
 
 	if (skb->pkt_type == PACKET_OTHERHOST)
@@ -448,7 +449,14 @@ static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!po)
 		goto drop;
 
-	return sk_receive_skb(sk_pppox(po), skb, 0);
+	sk = sk_pppox(po);
+	if (sk->sk_state & PPPOX_BOUND) {
+		ppp_input(&po->chan, skb);
+		sock_put(sk);
+		return NET_RX_SUCCESS;
+	}
+
+	return sk_receive_skb(sk, skb, 0);
 
 drop:
 	kfree_skb(skb);
-- 
2.43.0

[PATCH net-next 3/3] ppp: synchronize netstats updates

[Qingfang Deng]

The PPP receive path can now run concurrently across CPUs (after converting
rlock to rwlock in an earlier patch). This may lead to data races on
net_device->stats.

Convert all stats updates in both transmit and receive paths to use the
DEV_STATS_INC() macro, which updates stats atomically.

Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
 drivers/net/ppp/ppp_generic.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index f0f8a75e3aef..4e787d1823c1 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -1478,7 +1478,7 @@ ppp_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
  outf:
 	kfree_skb(skb);
-	++dev->stats.tx_dropped;
+	DEV_STATS_INC(dev, tx_dropped);
 	return NETDEV_TX_OK;
 }
 
@@ -1851,7 +1851,7 @@ ppp_send_frame(struct ppp *ppp, struct sk_buff *skb)
 
  drop:
 	kfree_skb(skb);
-	++ppp->dev->stats.tx_errors;
+	DEV_STATS_INC(ppp->dev, tx_errors);
 }
 
 /*
@@ -2134,7 +2134,7 @@ static int ppp_mp_explode(struct ppp *ppp, struct sk_buff *skb)
 	spin_unlock(&pch->downl);
 	if (ppp->debug & 1)
 		netdev_err(ppp->dev, "PPP: no memory (fragment)\n");
-	++ppp->dev->stats.tx_errors;
+	DEV_STATS_INC(ppp->dev, tx_errors);
 	++ppp->nxseq;
 	return 1;	/* abandon the frame */
 }
@@ -2296,7 +2296,7 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 	if (!ppp_decompress_proto(skb)) {
 		kfree_skb(skb);
 		if (pch->ppp) {
-			++pch->ppp->dev->stats.rx_length_errors;
+			DEV_STATS_INC(pch->ppp->dev, rx_length_errors);
 			ppp_receive_error(pch->ppp);
 		}
 		goto done;
@@ -2367,7 +2367,7 @@ ppp_receive_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 static void
 ppp_receive_error(struct ppp *ppp)
 {
-	++ppp->dev->stats.rx_errors;
+	DEV_STATS_INC(ppp->dev, rx_errors);
 	if (ppp->vj)
 		slhc_toss(ppp->vj);
 }
@@ -2634,7 +2634,7 @@ ppp_receive_mp_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 	 */
 	if (seq_before(seq, ppp->nextseq)) {
 		kfree_skb(skb);
-		++ppp->dev->stats.rx_dropped;
+		DEV_STATS_INC(ppp->dev, rx_dropped);
 		ppp_receive_error(ppp);
 		return;
 	}
@@ -2670,7 +2670,7 @@ ppp_receive_mp_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 		if (pskb_may_pull(skb, 2))
 			ppp_receive_nonmp_frame(ppp, skb);
 		else {
-			++ppp->dev->stats.rx_length_errors;
+			DEV_STATS_INC(ppp->dev, rx_length_errors);
 			kfree_skb(skb);
 			ppp_receive_error(ppp);
 		}
@@ -2776,7 +2776,7 @@ ppp_mp_reconstruct(struct ppp *ppp)
 		if (lost == 0 && (PPP_MP_CB(p)->BEbits & E) &&
 		    (PPP_MP_CB(head)->BEbits & B)) {
 			if (len > ppp->mrru + 2) {
-				++ppp->dev->stats.rx_length_errors;
+				DEV_STATS_INC(ppp->dev, rx_length_errors);
 				netdev_printk(KERN_DEBUG, ppp->dev,
 					      "PPP: reconstructed packet"
 					      " is too long (%d)\n", len);
@@ -2831,7 +2831,7 @@ ppp_mp_reconstruct(struct ppp *ppp)
 					      "  missed pkts %u..%u\n",
 					      ppp->nextseq,
 					      PPP_MP_CB(head)->sequence-1);
-			++ppp->dev->stats.rx_dropped;
+			DEV_STATS_INC(ppp->dev, rx_dropped);
 			ppp_receive_error(ppp);
 		}
 
-- 
2.43.0

Re: [PATCH net-next 3/3] ppp: synchronize netstats updates

[Andrew Lunn]

On Wed, Jun 25, 2025 at 11:40:20AM +0800, Qingfang Deng wrote:
> The PPP receive path can now run concurrently across CPUs (after converting
> rlock to rwlock in an earlier patch). This may lead to data races on
> net_device->stats.
> 
> Convert all stats updates in both transmit and receive paths to use the
> DEV_STATS_INC() macro, which updates stats atomically.

Do you have benchmark numbers for these changes? How big an
improvement does it makes?

https://elixir.bootlin.com/linux/v6.15.3/source/include/linux/netdevice.h#L5549

/* Note: Avoid these macros in fast path, prefer per-cpu or per-queue counters. */
#define DEV_STATS_INC(DEV, FIELD) atomic_long_inc(&(DEV)->stats.__##FIELD)

As far as i can see, you only use DEV_STATS_INC() in error paths. It
might be worth mentioning this. As the comment says, you don't want to
use it on the hot path.


    Andrew

---
pw-bot: cr