netdev - Re: [Intel-wired-lan] [PATCH iwl-net v3 4/6] idpf: replace flow scheduling buffer ring with buffer pool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d8a1978b-ca4c-4912-acf8-bcb6c921a7eb@intel.com>
Date: Mon, 4 Aug 2025 19:02:57 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Joshua Hay <joshua.a.hay@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>, Luigi Rizzo
	<lrizzo@...gle.com>, Brian Vazquez <brianvv@...gle.com>, Madhu Chittim
	<madhu.chittim@...el.com>, Aleksandr Loktionov
	<aleksandr.loktionov@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-net v3 4/6] idpf: replace flow
 scheduling buffer ring with buffer pool

From: Joshua Hay <joshua.a.hay@...el.com>
Date: Fri, 25 Jul 2025 11:42:21 -0700

> Replace the TxQ buffer ring with one large pool/array of buffers (only
> for flow scheduling). This eliminates the tag generation and makes it
> impossible for a tag to be associated with more than one packet.

[...]

> -static bool idpf_tx_clean_buf_ring(struct idpf_tx_queue *txq, u16 compl_tag,
> -				   struct libeth_sq_napi_stats *cleaned,
> -				   int budget)
> +static bool idpf_tx_clean_bufs(struct idpf_tx_queue *txq, u32 buf_id,
> +			       struct libeth_sq_napi_stats *cleaned,
> +			       int budget)
>  {
> -	u16 idx = compl_tag & txq->compl_tag_bufid_m;
>  	struct idpf_tx_buf *tx_buf = NULL;
>  	struct libeth_cq_pp cp = {
>  		.dev	= txq->dev,
>  		.ss	= cleaned,
>  		.napi	= budget,
>  	};
> -	u16 ntc, orig_idx = idx;
> -
> -	tx_buf = &txq->tx_buf[idx];
> -
> -	if (unlikely(tx_buf->type <= LIBETH_SQE_CTX ||
> -		     idpf_tx_buf_compl_tag(tx_buf) != compl_tag))
> -		return false;
>  
> +	tx_buf = &txq->tx_buf[buf_id];
>  	if (tx_buf->type == LIBETH_SQE_SKB) {
>  		if (skb_shinfo(tx_buf->skb)->tx_flags & SKBTX_IN_PROGRESS)
>  			idpf_tx_read_tstamp(txq, tx_buf->skb);
>  
>  		libeth_tx_complete(tx_buf, &cp);
> +		idpf_post_buf_refill(txq->refillq, buf_id);
>  	}
>  
> -	idpf_tx_clean_buf_ring_bump_ntc(txq, idx, tx_buf);
> +	while (idpf_tx_buf_next(tx_buf) != IDPF_TXBUF_NULL) {
> +		u16 buf_id = idpf_tx_buf_next(tx_buf);

This line adds a new -Wshadow warning. This function has an argument
named 'buf_id' and here you declare a variable with the same name.
While -Wshadow gets enabled only with W=2, it would be nice if you don't
introduce it anyway.
If I understand the code correctly, you can just remove that `u16` since
you don't use the former buf_id at this point anyway.

>  
> -	while (idpf_tx_buf_compl_tag(tx_buf) == compl_tag) {
> +		tx_buf = &txq->tx_buf[buf_id];
>  		libeth_tx_complete(tx_buf, &cp);
> -		idpf_tx_clean_buf_ring_bump_ntc(txq, idx, tx_buf);
> +		idpf_post_buf_refill(txq->refillq, buf_id);
>  	}

Thanks,
Olek