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

[Jesper Dangaard Brouer <hawk@kernel.org>]

On 08/07/2025 02.43, Jakub Kicinski wrote:
> 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?
> 

Good catch, yes the brackets are unnecessary, will remove in V5.

>> @@ -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 like it, this is a good description, thanks! :-)

> 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))

Agreed, will fix in V5.

> 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
> 

Sure I will adjust code.

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

--Jesper