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

[Ratheesh Kannoth]

On 2026-10-05 at 13:44:40, David Laight (david.laight.linux@xxxxxxxxx) wrote:
> On Mon, 5 Oct 2026 08:28:17 +0530
> Ratheesh Kannoth <rkannoth@xxxxxxxxxxx> wrote:
>
> > On 2026-10-02 at 15:07:52, David Laight (david.laight.linux@xxxxxxxxx) wrote:
> >
> > 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

Understood on cost and patch size. Before the series, we had a single shared flags word
updated with non-atomic |= / &=~ operations.

When we reviewed accessors across all bits, we found several that are written from rtnl or mbox
workqueues but read on other CPUs without rtnl protection: INTF_DOWN (NAPI, XDP, QoS—the driver
already notes it may be checked on any CPU), PORT_UP (rep port-state mbox up vs. open/link
handling), ADPTV_INT_COAL_ENABLED (ethtool vs. NAPI), and TC_MARK_ENABLED (flower vs. RX/TX). With
eswitch, the rep sync mask bits (MCAM_ENTRIES_ALLOC, NTUPLE_SUPPORT, TC_FLOWER_SUPPORT,
REP_VF_INITIALIZED) add another writer path on the same word.

Converting accessors to atomic bitops makes updates and lockless tests well-defined on that
shared word. We're not claiming every capability flag has two live writers—the driver-wide
change is for consistent concurrency on the bitmap, not because mqprio alone touched every bit.

On expense: most updates are rare, and the recurring cost is test_bit on a few hot paths versus
a mask test, which we consider acceptable for defined behavior. This change is also a
straightforward, mechanical string replacement. Happy to discuss if you'd rather optimize for
minimal diff instead.

>
> >
> > >
>