Re: [PATCH net-next v3] ppp: enable TX scatter-gather

[Paolo Abeni <pabeni@redhat.com>]

On 1/23/26 2:42 AM, Qingfang Deng wrote:
> PPP channels using chan->direct_xmit prepend the PPP header to a skb and
> call dev_queue_xmit() directly. In this mode the skb does not need to be
> linear, but the PPP netdevice currently does not advertise
> scatter-gather features, causing unnecessary linearization and
> preventing GSO.
> 
> Enable NETIF_F_SG and NETIF_F_FRAGLIST on PPP devices and add an
> .ndo_fix_features() callback to disable them when the underlying PPP
> channel is not using direct_xmit (i.e. IFF_NO_QUEUE is not set in
> priv_flags). This allows the networking core to pass non-linear skbs
> directly only when it is safe to do so.
> 
> PPP compressors still require linear skbs, and their states can change
> at runtime. In order to avoid races, instead of toggling features, call
> skb_linearize() before passing buffers to them. Compressors are uncommon
> on high-speed links, so this does not affect the fast path.
> 
> Signed-off-by: Qingfang Deng <dqfext@gmail.com>
> ---
> v2 -> v3:
>  toggle features only if the underlying ppp_channel changes. Call
>   skb_linearize() if compressors are in use.
>  - https://lore.kernel.org/linux-ppp/20251103031501.404141-1-dqfext@gmail.com/
> 
>  drivers/net/ppp/ppp_generic.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index f9f0f16c41d1..d706a175cea9 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -1545,6 +1545,16 @@ ppp_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats64)
>  	dev_fetch_sw_netstats(stats64, dev->tstats);
>  }
>  
> +static netdev_features_t
> +ppp_fix_features(struct net_device *dev, netdev_features_t features)
> +{
> +	/* Don't advertise SG/FRAGLIST when IFF_NO_QUEUE is absent */
> +	if (!(dev->priv_flags & IFF_NO_QUEUE))
> +		features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);

I spent a little time trying to understanding the logic here and I think
that enabling features depending on IFF_NO_QUEUE is fragile at best.

It looks like that the IFF_NO_QUEUE bit is an inconsistent state for
multilink devices using different type of channels.
Moreover the user-space could attaching a qdisc to the ppp device after
channel initialization.

Instead you could always expose the features and linearize as needed
when transmitting on !direct_xmit channel; no need to touch the
individual channel implementation, you could do such check before
calling the ops->start_xmit() calls (possibly creating a new
wrapper/helper for that).

/P