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

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

On Wed, Nov 22, 2023 at 05:48:45PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>

snip

> > 
> > 
> > Fixes: 7ff6ad107588 ("cxl/memdev: Add trigger_poison_list sysfs attribute")
> > Fixes: f0832a586396 ("cxl/region: Provide region info to the cxl_poison trace event")
> > Fixes: d2fbc4865802 ("cxl/memdev: Add support for the Inject Poison mailbox command")
> > Fixes: 9690b07748d1 ("cxl/memdev: Add support for the Clear Poison mailbox command")
> 
> I think this is an indication that the fixes would benefit from being
> broken up into at least 2 commits so that the specific side effect of
> each can be commented upon.
> 

Agree. I've split into 2 patches and expanded the impact in each commit
message.

> For example:
> 
> - Fix walking committed decoders in cxl_trigger_poison_list()

That's not where I found the lock issue. When walking committed
decoders, the region_rwsem was held. It is when we think there
are no regions mapped, and collect by memdev only, that I found
a gap.

> 
> - Fix walking dpa to region lookups in cxl_{inject,clear}_poison()

We're probably in sync here, albeit different words.

> 
> ...look like 2 separate topics in this combined patch.
> 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/memdev.c | 18 +++++++++---------
> >  drivers/cxl/core/region.c |  5 -----
> >  2 files changed, 9 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > index fc5c2b414793..961da365b097 100644
> > --- a/drivers/cxl/core/memdev.c
> > +++ b/drivers/cxl/core/memdev.c
> > @@ -227,9 +227,8 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
> >  	if (!port || !is_cxl_endpoint(port))
> >  		return -EINVAL;
> >  
> > -	rc = down_read_interruptible(&cxl_dpa_rwsem);
> > -	if (rc)
> > -		return rc;
> 
> What the rationale for dropping interruptible, it seems appropriate here
> since this function is directly servicing a debugfs trigger and maybe
> someone gets tired of waiting.
> 

This one is the sysfs, not debugfs, trigger. They shouldn't get tired of
waiting as it is only reading static info - the existing poison list of 
the device.

However, I did put back the _interruptible...for now.

I am not clear on how it all trickles down if this gets interrupted and
need to understand that further. We are trying to maintain a poison
state, and prevent partial reads of poison lists. That is not part of 
this patch, so I will study that some more and come back at it if I 
still think it's an issue.


> > +	down_read(&cxl_region_rwsem);
> > +	down_read(&cxl_dpa_rwsem);
> >  

snip

> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2467,10 +2467,6 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port)
> >  	struct cxl_poison_context ctx;
> >  	int rc = 0;
> >  
> > -	rc = down_read_interruptible(&cxl_region_rwsem);
> > -	if (rc)
> > -		return rc;
> > -
> >  	ctx = (struct cxl_poison_context) {
> >  		.port = port
> >  	};
> > @@ -2480,7 +2476,6 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port)
> >  		rc = cxl_get_poison_unmapped(to_cxl_memdev(port->uport_dev),
> >  					     &ctx);
> >  
> > -	up_read(&cxl_region_rwsem);
> >  	return rc;
> 
> This hunk deserves to be called out that region locking is being
> upleveled as part of topic 1, and this reinforces splitting the 2 topics
> into 2 patches.

done.

> 
> Keep the _interruptible versions throughout, if you want to drop
> interruptible that should be a separate follow-on behavior change patch.
> The need to keep _interruptible also obviates conversion to cleanup.h
> helpers for now.

done.