Re: PCIe enable device races (Was: [PATCH v3] PCI: Data corruption happening due to race condition)

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2018-08-16 at 14:28 +0200, Lukas Wunner wrote:
> On Thu, Aug 16, 2018 at 07:52:06AM +1000, Benjamin Herrenschmidt wrote:
> > I would also like to use this rather than the bitmap atomics for is_added
> > etc... (Hari's fix) in the long run. Atomics aren't significantly cheaper
> > and imho makes thing even messier.
> 
> PCI device enablement isn't a hotpath, so the performance saving of
> acquire/release memory barriers vs. full memory barriers is hardly
> a strong argument.

My argument is not about performance. Fine grained atomics are a mess,
and make you lose the big picture. The problem isn't so much that
indiviual bits need to avoid low level RMW races, but overall higher
level bnehaviours need to be synchronized, such as the enable race
above.

> Atomic bitops have the advantage of being usable in atomic context,
> unlike a struct mutex.  And they don't require any spinning if the
> variable is accessed concurrently.

And why on earth would you need to use pci_enable_device or manipulate
is_added in an atomic context ?

Atomic bitops are a giant race-o-matic and experience shows that 99% of
the time, people using them insteaf of standard locking are getting it
wrong.

The PCI code is already messy enough, let's not "fix" the broken
synchronization accross the board in an even messier way.

> Last not least an atomic bitop needs just 1 line of code versus 3 lines
> of code to acquire a lock, update the variable and release the lock.
> "Elegance is not optional."

Sorry, atomics isn't elegance, it's broken crap. The flags dont' live
in isolation. They relate to an overall function. For example
is_busmaster relates to the actual setting of the bus master bit in the
command register. Those two needs to be updated together atomically,
you need a lock or a mutex. Not an atomic.

This is true of about everything else.

> That said, the race *you're* dealing with appears to be distinct from
> Hari's in that a lock is unavoidable because (AFAIUI) atomic_inc_return()
> is called before enablement actually happens, yet both needs to happen
> atomically.

Every single time one of these flags represent more than the flag
itself, but some other state that is being modified or updates, then
you need a lock to cover both the flag and the other state.

So if we are going to create a sane locking mechanism to protect the
various bits of pci_dev (not just enable), let's use it for everything
shall we ? At least everything that doesn't need to be manipulated in
atomic context.

That also possibly include removing some of the ad-hoc locks in various
parts of pcie/* etc...

Ben.

> Thanks,
> 
> Lukas