Re: [PATCH v18 net-next 0/2] octeontx2: mqprio bandwidth offload for NIX TX schedulers

[David Laight <david.laight.linux@gmail.com>]

On Mon, 5 Oct 2026 08:28:17 +0530
Ratheesh Kannoth <rkannoth@marvell.com> wrote:

> On 2026-10-02 at 15:07:52, David Laight (david.laight.linux@gmail.com) wrote:
> > > Patch 1 converts PF/VF and representor flag access to atomic bitops.
> > > Patch 2 depends on it for safe OTX2_FLAG_INTF_DOWN and OTX2_FLAG_PORT_UP
> > > updates on asynchronous mbox paths and during the mqprio netdev bounce.  
> >
> > Can't you just move those two flags to a separate structure member?
> > In at least one place the code separately clears one and sets the other.
> > That makes me think it should a a three-valued state not two bits.
> >
> > That would save all the expensive locked operations.  
> 
> Thanks for your review.
> 
> I agree that atomizing the entire flags bitmap is broader than strictly required for the
> mqprio/mbox concurrency: the cross-CPU hazard is otx2_sync_flags_from_rep() doing a non-atomic
> read-modify-write on nic->flags while other CPUs update bits in the same word. Relocating
> OTX2_FLAG_INTF_DOWN and OTX2_FLAG_PORT_UP (or restricting rep sync so it cannot clobber those
> bits) would be a narrower approach than converting every OTX2_FLAG_* accessor to
> set_bit()/clear_bit().
> 
> On the three-valued model: INTF_DOWN and PORT_UP are not a single FSM in the driver today.
> INTF_DOWN reflects netdev teardown and is consulted from NAPI completion etc.
> PORT_UP is set and cleared from rep RVU_EVENT_PORT_STATE in the mbox up-handler and
> suppresses a duplicate carrier/queue bring-up in otx2_handle_link_event() when rep has already
> applied port state. Combinations such as INTF_DOWN set after otx2_stop() while PORT_UP remains
> set are deliberate, so folding the two bits into one enum would need a seperate work
> and review beyond this series.
> 
> please note that this restructuring feels somewhat orthogonal to the goals of the
> current series. Patch 1 focuses on replacing the non-atomic |=/&=~ operations on the
> stop/open and mbox paths with atomic bitops for INTF_DOWN/PORT_UP, which patch 2's netdev
> bounce depends on. The rep-flag sync behavior in otx2_sync_flags_from_rep() is a related but
> separate concern, and I'd prefer to address the lifecycle-bit layout and sync logic in a
> dedicated follow-up rather than expand the scope of this patch series.

Right, but the atomic updates are are far more expensive than the non-atomic ones.
They really are best avoided unless you really need to change/test multiple bits
or need to limit the size of the data area.
The patch is likely to be smaller if you remove the UP/DOWN bits from the bitmap
since it will change far less code.

David
 
> 
> >