Re: [RFC PATCH net v3 1/1] af_packet: avoid waiting for TPACKET_V3 copies at retirement

[Willem de Bruijn]

Jingxuan Xie wrote:
> From: jingxuanxie <jingxuanxie@xxxxxxxxxxxx>
> 
> TPACKET_V3 retires a full or expired block while holding
> sk_receive_queue.lock. prb_retire_current_block() takes
> blk_fill_in_prog_lock for write to wait for in-flight copy_bits()
> before publishing the block.
> 
> On PREEMPT_RT a copier can be preempted while holding the read lock.
> The packet path or the retirement timer then blocks on the write lock
> while holding sk_receive_queue.lock, and the receive path resumes only
> after the copier is scheduled again.
> 
> Try to take the write lock without blocking. If a copy is still in
> progress, leave the current block and its metadata untouched:
> 
> - the retirement timer re-arms itself and retries after the configured
>   interval;
> - packet allocation returns NULL, so tpacket_rcv() drops this capture
>   record and accounts it in tp_drops, while the original packet remains
>   in the network stack;
> - a later packet can retire the block once the copier has finished.
> 
> Do not dispatch the next block while the current block is still open.
> The existing lock order and completion ordering are preserved, and no
> new state or worker is introduced. The trade-off is a higher capture
> drop rate while a copier is preempted; original traffic is unaffected.
> 
> Fixes: 632ca50f2cbd ("af_packet: TPACKET_V3: replace busy-wait loop")
> Assisted-by: LLM
> Signed-off-by: Jingxuan Xie <jingxuanxie@xxxxxxxxxxxx>

No need for a cover letter for a single patch.

And a v4 will be needed, because this is sent as RFC.

> ---
>  net/packet/af_packet.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index cf6429f09477..8fd6372ab59b 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -200,8 +200,8 @@ static void packet_increment_head(struct packet_ring_buffer *buff);
>  static int prb_curr_blk_in_use(struct tpacket_block_desc *);
>  static void *prb_dispatch_next_block(struct tpacket_kbdq_core *,
>  			struct packet_sock *);
> -static void prb_retire_current_block(struct tpacket_kbdq_core *,
> -		struct packet_sock *, unsigned int status);
> +static bool prb_retire_current_block(struct tpacket_kbdq_core *,
> +				     struct packet_sock *, unsigned int status);

no unrelated style changes

>  static int prb_queue_frozen(struct tpacket_kbdq_core *);
>  static void prb_open_block(struct tpacket_kbdq_core *,
>  		struct tpacket_block_desc *);
> @@ -711,8 +711,9 @@ static enum hrtimer_restart prb_retire_rx_blk_timer_expired(struct hrtimer *t)
>  	 *
>  	 */
>  	if (BLOCK_NUM_PKTS(pbd)) {
> -		/* Waiting for skb_copy_bits to finish... */
> -		write_lock(&pkc->blk_fill_in_prog_lock);
> +		/* Retry later if a packet copy is still in progress. */
> +		if (!write_trylock(&pkc->blk_fill_in_prog_lock))
> +			goto refresh_timer;

Since this is only a problem on RT, and it introduces drops,
consider limiting the switch from write_lock to write_trylock to only RT.

>  		write_unlock(&pkc->blk_fill_in_prog_lock);
>  	}
>  
> @@ -738,6 +739,7 @@ static enum hrtimer_restart prb_retire_rx_blk_timer_expired(struct hrtimer *t)
>  		}
>  	}
>  
> +refresh_timer:
>  	hrtimer_forward_now(&pkc->retire_blk_timer, pkc->interval_ktime);
>  	spin_unlock(&po->sk.sk_receive_queue.lock);
>  	return HRTIMER_RESTART;
> @@ -946,8 +948,8 @@ static void *prb_dispatch_next_block(struct tpacket_kbdq_core *pkc,
>  	return (void *)pkc->nxt_offset;
>  }
>  
> -static void prb_retire_current_block(struct tpacket_kbdq_core *pkc,
> -		struct packet_sock *po, unsigned int status)
> +static bool prb_retire_current_block(struct tpacket_kbdq_core *pkc,
> +				     struct packet_sock *po, unsigned int status)

again, unnecessary style changes

>  {
>  	struct tpacket_block_desc *pbd = GET_CURR_PBLOCK_DESC_FROM_CORE(pkc);
>  
> @@ -963,13 +965,14 @@ static void prb_retire_current_block(struct tpacket_kbdq_core *pkc,
>  		 * the timer-handler already handled this case.
>  		 */
>  		if (!(status & TP_STATUS_BLK_TMO)) {
> -			/* Waiting for skb_copy_bits to finish... */
> -			write_lock(&pkc->blk_fill_in_prog_lock);
> +			/* Do not wait for copies while holding the queue lock. */
> +			if (!write_trylock(&pkc->blk_fill_in_prog_lock))
> +				return false;
>  			write_unlock(&pkc->blk_fill_in_prog_lock);
>  		}
>  		prb_close_block(pkc, pbd, po, status);
> -		return;
>  	}
> +	return true;
>  }
>  
>  static int prb_curr_blk_in_use(struct tpacket_block_desc *pbd)
> @@ -1097,7 +1100,8 @@ static void *__packet_lookup_frame_in_block(struct packet_sock *po,
>  	}
>  
>  	/* Ok, close the current block */
> -	prb_retire_current_block(pkc, po, 0);
> +	if (!prb_retire_current_block(pkc, po, 0))
> +		return NULL;
>  
>  	/* Now, try to dispatch the next block */
>  	curr = (char *)prb_dispatch_next_block(pkc, po);