Re: [PATCH] cxl/core: Hold the region rwsem during poison ops

[Alison Schofield <alison.schofield@intel.com>]

On Thu, Nov 16, 2023 at 02:14:37PM -0800, Davidlohr Bueso wrote:
> On Mon, 13 Nov 2023, alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Commit 458ba8189cb4 ("cxl: Add cxl_decoders_committed() helper")
> > added a lockdep_assert_held() to make sure all callers hold the
> > region state stable while doing work that depends on the number
> > of committed decoders.
> 
> Unrelated, but that kind of helper would be better as a static
> inline in a header file.
> 
> > 
> > That lockdep assert triggered in poison list, inject, and clear
> > functions highlighting a gap between region attach and decoder
> > commit where holding the dpa_rwsem is not enough to assure that
> > a DPA is not added to a region. In such a case, if poison is
> > found in at that DPA, the trace event omits the region info
> > that users expect.
> > 
> > Close the gap by snapshotting an unchangeable region state during
> > all poison ops. Hold the region_rwsem in all the places that hold
> > the dpa_rwsem rather than in the region specific function only.
> 
> Makes sense and lock oder is consistent with that of attach_target().
> I do think that the interruptible semantics should be kept considering
> this is from sysfs/debugfs.

Good eye, bad me!  It was intentional to remove the interruptible,
but I should have noted it in the commit log.

At the point the lock is taken, the driver is committed to completing
the action. It is not interruptible once begun. This notion of a poison
state was first introduced here:
d0abf5787adc ("cxl/mbox: Initialize the poison state")

The basic premise is that the driver will synchronize reads of poison
so as not to return incomplete lists.

> 
> Thanks,
> Davidlohr