Re: [PATCH net 6/8] ice: improve updating ice_{t, r}x_ring::xsk_pool

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Tue, Jul 09, 2024 at 06:45:24PM -0700, Jakub Kicinski wrote:
> On Mon,  8 Jul 2024 15:14:12 -0700 Tony Nguyen wrote:
> > @@ -1556,7 +1556,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> >  		 * comparison in the irq context instead of many inside the
> >  		 * ice_clean_rx_irq function and makes the codebase cleaner.
> >  		 */
> > -		cleaned = rx_ring->xsk_pool ?
> > +		cleaned = READ_ONCE(rx_ring->xsk_pool) ?
> >  			  ice_clean_rx_irq_zc(rx_ring, budget_per_ring) :
> >  			  ice_clean_rx_irq(rx_ring, budget_per_ring);
> >  		work_done += cleaned;
> 
> 
> > @@ -832,8 +839,8 @@ ice_add_xsk_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *first,
> >   */
> >  int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> >  {
> > +	struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
> >  	unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> > -	struct xsk_buff_pool *xsk_pool = rx_ring->xsk_pool;
> >  	u32 ntc = rx_ring->next_to_clean;
> >  	u32 ntu = rx_ring->next_to_use;
> >  	struct xdp_buff *first = NULL;
> 
> This looks suspicious, you need to at least explain why it's correct.
> READ_ONCE() means one access per critical section, usually.
> You access it at least twice in a single NAPI pool.

Hey after break! Comebacks are tough, vacation was followed by flu so bear
with me please...

Actually xsk_pool *can* be accessed multiple times during the refill of HW
Rx ring (at the end of napi poll Rx side). I thought it would be safe to
follow the scheme of xdp prog pointer handling, where we read it from ring
once per napi loop then work on local pointer.

Goal of this commit was to prevent compiler from code reoder such as NAPI
is launched before update of xsk_buff_pool pointer which is achieved with
WRITE_ONCE()/synchronize_net() pair. Then per my understanding single
READ_ONCE() within NAPI was sufficient, the one that makes the decision
which Rx routine should be called (zc or standard one). Given that bh are
disabled and updater respects RCU grace period IMHO pointer is valid for
current NAPI cycle.

If you're saying it's not correct and each and every xsk_pool reference
within NAPI has to be decorated with READ_ONCE() then so is the xdp_prog
pointer, but I'd like to hear more about this.