Re: [Intel-wired-lan] [PATCH iwl-next v2 3/4] igb: add AF_XDP zero-copy Rx support

From: Simon Horman <simon.horman@corigine.com>
To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
Cc: "Jesper Dangaard Brouer" <hawk@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	netdev@vger.kernel.org,
	"Jonathan Lemon" <jonathan.lemon@gmail.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Jesse Brandeburg" <jesse.brandeburg@intel.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Björn Töpel" <bjorn@kernel.org>,
	"Eric Dumazet" <edumazet@google.com>,
	"Tony Nguyen" <anthony.l.nguyen@intel.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	intel-wired-lan@lists.osuosl.org, bpf@vger.kernel.org,
	"Paolo Abeni" <pabeni@redhat.com>,
	"David S . Miller" <davem@davemloft.net>,
	"Magnus Karlsson" <magnus.karlsson@intel.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 3/4] igb: add AF_XDP zero-copy Rx support
Date: Thu, 13 Jul 2023 10:29:25 +0100	[thread overview]
Message-ID: <ZK/D9c0TYzH30wqf@corigine.com> (raw)
In-Reply-To: <20230711114705.22428-4-sriram.yagnaraman@est.tech>

On Tue, Jul 11, 2023 at 01:47:04PM +0200, Sriram Yagnaraman wrote:
> Add support for AF_XDP zero-copy receive path.
> 
> When AF_XDP zero-copy is enabled, the rx buffers are allocated from the
> xsk buff pool using igb_alloc_rx_buffers_zc.
> 
> Use xsk_pool_get_rx_frame_size to set SRRCTL rx buf size when zero-copy
> is enabled.
> 
> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>

Hi Sriram,

> +bool igb_alloc_rx_buffers_zc(struct igb_ring *rx_ring, u16 count)
> +{
> +	union e1000_adv_rx_desc *rx_desc;
> +	u32 nb_buffs_extra = 0, nb_buffs;
> +	u16 ntu = rx_ring->next_to_use;
> +	u16 total_count = count;
> +	struct xdp_buff **xdp;
> +
> +	rx_desc = IGB_RX_DESC(rx_ring, ntu);
> +	xdp = &rx_ring->rx_buffer_info_zc[ntu];
> +
> +	if (ntu + count >= rx_ring->count) {
> +		nb_buffs_extra = igb_fill_rx_descs(rx_ring->xsk_pool, xdp,
> +						   rx_desc,
> +						   rx_ring->count - ntu);
> +		if (nb_buffs_extra != rx_ring->count - ntu) {
> +			ntu += nb_buffs_extra;
> +			goto exit;

nb_buffs is uninitialised here...

> +		}
> +		rx_desc = IGB_RX_DESC(rx_ring, 0);
> +		xdp = rx_ring->rx_buffer_info_zc;
> +		ntu = 0;
> +		count -= nb_buffs_extra;
> +	}
> +
> +	nb_buffs = igb_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
> +	ntu += nb_buffs;
> +	if (ntu == rx_ring->count)
> +		ntu = 0;
> +
> +	/* clear the length for the next_to_use descriptor */
> +	rx_desc = IGB_RX_DESC(rx_ring, ntu);
> +	rx_desc->wb.upper.length = 0;
> +
> +exit:
> +	if (rx_ring->next_to_use != ntu) {
> +		rx_ring->next_to_use = ntu;
> +
> +		/* Force memory writes to complete before letting h/w
> +		 * know there are new descriptors to fetch.  (Only
> +		 * applicable for weak-ordered memory model archs,
> +		 * such as IA-64).
> +		 */
> +		wmb();
> +		writel(ntu, rx_ring->tail);
> +	}
> +
> +	return total_count == (nb_buffs + nb_buffs_extra);

But it is used here.

...

The following will tell you about this problem:
- Smatch
- clang-16 W=1 build [-Wsometimes-uninitialized]
- gcc-12 build with -Wmaybe-uninitialized
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan