Re: [PATCH net-next V4] net: track pfmemalloc drops via SKB_DROP_REASON_PFMEMALLOC

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 02 Jul 2025 15:59:19 +0200 Jesper Dangaard Brouer wrote:
> Add a new SKB drop reason (SKB_DROP_REASON_PFMEMALLOC) to track packets
> dropped due to memory pressure. In production environments, we've observed
> memory exhaustion reported by memory layer stack traces, but these drops
> were not properly tracked in the SKB drop reason infrastructure.
> 
> While most network code paths now properly report pfmemalloc drops, some
> protocol-specific socket implementations still use sk_filter() without
> drop reason tracking:
> - Bluetooth L2CAP sockets
> - CAIF sockets
> - IUCV sockets
> - Netlink sockets
> - SCTP sockets
> - Unix domain sockets

> @@ -1030,10 +1030,8 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>  	}
>  
>  	if (tfile->socket.sk->sk_filter &&
> -	    sk_filter(tfile->socket.sk, skb)) {
> -		drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
> +	    (sk_filter_reason(tfile->socket.sk, skb, &drop_reason)))

why the outside brackets?

> @@ -591,6 +592,10 @@ enum skb_drop_reason {
>  	 * non conform CAN-XL frame (or device is unable to receive CAN frames)
>  	 */
>  	SKB_DROP_REASON_CANXL_RX_INVALID_FRAME,
> +	/**
> +	 * @SKB_DROP_REASON_PFMEMALLOC: dropped when under memory pressure

I guess kinda, but in practice not very precise?

How about: packet allocated from memory reserve reached a path or
socket not eligible for use of memory reserves.

I could be misremembering the meaning of "memory reserve" TBH.

> +	 */
> +	SKB_DROP_REASON_PFMEMALLOC,
>  	/**
>  	 * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which
>  	 * shouldn't be used as a real 'reason' - only for tracing code gen

> -	if (unlikely(sk_add_backlog(sk, skb, limit))) {
> +	if (unlikely((err = sk_add_backlog(sk, skb, limit)))) {

I understand the else if () case but here you can simply:

	err = sk_add_backlog(sk, skb, limit);
	if (unlikely(err))

no need to make checkpatch upset.

> @@ -162,7 +163,7 @@ static int rose_state3_machine(struct sock *sk, struct sk_buff *skb, int framety
>  		rose_frames_acked(sk, nr);
>  		if (ns == rose->vr) {
>  			rose_start_idletimer(sk);
> -			if (sk_filter_trim_cap(sk, skb, ROSE_MIN_LEN) == 0 &&
> +			if (sk_filter_trim_cap(sk, skb, ROSE_MIN_LEN, &dr) == 0 &&

let's switch to negation rather than comparing to 0 while at it?
otherwise we run over 80 chars

>  			    __sock_queue_rcv_skb(sk, skb) == 0) {
>  				rose->vr = (rose->vr + 1) % ROSE_MODULUS;
>  				queued = 1;
-- 
pw-bot: cr