Re: [dpdk-dev] [EXT] [PATCH v3 6/6] spinlock: ticket based to improve fairness

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

> > > > >
> > > > > On Thu, 2018-12-27 at 12:13 +0800, Gavin Hu wrote:
> > > > > > -----------------------------------------------------------
> > > > > > ----
> > > > > > ----
> > > > > > ---
> > > > > > From: Joyce Kong <joyce.kong@arm.com>
> > > > > >
> > > > > > The old implementation is unfair, some threads may take locks
> > > > > > aggressively
> > > > >
> > > > > I think, one issue here is x86 and ppc follows traditional
> > > > > spinlock and
> > > > > arm64 will be following ticket lock for spinlock implementation.
> > > > > This would change application behaviour on arm64 compared to
> > > > > x86
> > > > > and
> > > > > ppc.
> > > > >
> > > > > How about having a separate API for ticket lock? That would
> > > > > give, # application choice to use the locking strategy #
> > > > > application behaviour will be same across all arch.
> > > >
> > > > Ok, will do in v4 to have a new named rte_ticket_spinlock API.
> > >
> > > I would prefer rte_ticketlock_[lock/unlock/trylock/is_locked] name
> > > instead of rte_ticket_spinlock_lock etc to reduce the length of the
> > > API.
> >
> > NAK to adding new API for this.
> >
> > I want the best possible locks for all applications and all
> > architectures.
> > These should be called spinlock so there is no requirement for
> > application to change to get better performance. Why not just
> > implement the best algorithm across the board. Yes, this means
> > collaboration or working on the other guys architecture.
IMO, the ticket lock should be a separate API. Spin lock (as implemented today) and ticket lock have different characteristics in terms of behavior as well as resources required. For ex: spin lock might be sufficient for a low contention use case and it requires less number of cache lines. Ticket lock needs more number of cache lines and might be good for use cases with more contention. The decision to use spin lock vs ticket lock should be left to the application.

> 
> Then 6/6 patch needs to put on hold if every arch needs to make ticket lock
> as default spinlock lock strategy.
+1 for doing 6/6 as a separate patch.

> 
> How about following to make forward progress:
> 1) Introduce rte_ticketlock_[lock/unlock/trylock/is_locked] API now as
> experimental with default implementation
> 2) Provide a time line to switch every arch for optimized ticketlock
> implementation if needed.
> 3) Switch rte_ticketlock_ as rte_spinlock_ API.
> 4) Keep old version of spinlock as new API if some application does not need
> fairness between threads at the cost of light weight spinlock
> implementation.
> 
> I don't want arm64 to behave differently than other arch(s).