Re: [PATCH v3] cxl/region: Remove lock from memory notifier callback

[Ira Weiny]

Jonathan Cameron wrote:
> On Wed, 04 Sep 2024 09:47:54 -0500
> Ira Weiny <ira.weiny@xxxxxxxxx> wrote:
> 

[snip]

> > 
> > Link: https://lore.kernel.org/all/66b4cf539a79b_a36e829416@iweiny-mobl.notmuch/ [0]
> > Cc: Ying Huang <ying.huang@xxxxxxxxx>
> > Suggested-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Reviewed-by: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Reviewed-by: Ying Huang <ying.huang@xxxxxxxxx>
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> A few comments on looking at this again, but all things that apply equally to old
> code so maybe things for another day.

Yea this was solely a move of existing code to fix the locking issue.  I
did not evaluate the original code.  However...

[snip]

> >  }
> >  
> > +static void shutdown_notifiers(void *_cxlr)
> > +{
> > +	struct cxl_region *cxlr = _cxlr;
> > +
> > +	unregister_memory_notifier(&cxlr->memory_notifier);
> > +	unregister_mt_adistance_algorithm(&cxlr->adist_notifier);
> Flip order.
> 
> Makes zero real difference, but if we later end up with more to do
> here for some reason there may be ordering requirements that will
> care that this doesn't tear down in reverse of setup.

Generally I agree with you however, the memory and adist notifiers are
unrelated.  So failing to unwind in reverse order is a matter of taste and
is not required even if some other logic was introduced between the
registrations I don't see how this backwards order would be an issue.

> 
> Mind you, see below.
> 
> > +}
> > +
> >  static int cxl_region_probe(struct device *dev)
> >  {
> >  	struct cxl_region *cxlr = to_cxl_region(dev);
> > @@ -3418,6 +3412,18 @@ static int cxl_region_probe(struct device *dev)
> >  out:
> >  	up_read(&cxl_region_rwsem);
> >  
> > +	if (rc)
> > +		return rc;
> > +
> > +	cxlr->memory_notifier.notifier_call = cxl_region_perf_attrs_callback;
> > +	cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;
> > +	register_memory_notifier(&cxlr->memory_notifier);
> Can in theory fail.  Today that is EEXIST only but who knows in future.
> I think we should handle that and do two devm_add_action_or_reset() perhaps?
> 

First we should not fail the probe if this fails.

Second, nothing bad happens in unregister if the registration failed.
Therefore, register failing is benign and I don't see a need for the extra
action callback.

Ira

[snip]