Re: [PATCH 28/46] cxl/port: Move dport tracking to an xarray

From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <nvdimm@lists.linux.dev>,
	<linux-pci@vger.kernel.org>, <patches@lists.linux.dev>,
	<hch@lst.de>
Subject: Re: [PATCH 28/46] cxl/port: Move dport tracking to an xarray
Date: Sun, 10 Jul 2022 12:06:21 -0700	[thread overview]
Message-ID: <62cb232d48a38_3177ca294ac@dwillia2-xfh.notmuch> (raw)
In-Reply-To: <20220630101808.0000714f@Huawei.com>

Jonathan Cameron wrote:
> On Thu, 23 Jun 2022 21:19:32 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Reduce the complexity and the overhead of walking the topology to
> > determine endpoint connectivity to root decoder interleave
> > configurations.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Hi Dan,
> 
> A few minor comments inline around naming and also one query on why
> the refactor or reap_ports is connected to the xarray change.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/cxl/acpi.c      |  2 +-
> >  drivers/cxl/core/hdm.c  |  6 ++-
> >  drivers/cxl/core/port.c | 88 ++++++++++++++++++-----------------------
> >  drivers/cxl/cxl.h       | 12 +++---
> >  4 files changed, 51 insertions(+), 57 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 09fe92177d03..92ad1f359faf 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -197,7 +197,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >  	if (!bridge)
> >  		return 0;
> >  
> > -	dport = cxl_find_dport_by_dev(root_port, match);
> > +	dport = cxl_dport_load(root_port, match);
> 
> Load is kind of specific to the xarray.  I'd be tempted to keep it to
> original find naming.

ok.

> 
> 
> >  	if (!dport) {
> >  		dev_dbg(host, "host bridge expected and not found\n");
> >  		return 0;
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index c0164f9b2195..672bf3e97811 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -50,8 +50,9 @@ static int add_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >  int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
> >  {
> >  	struct cxl_switch_decoder *cxlsd;
> > -	struct cxl_dport *dport;
> > +	struct cxl_dport *dport = NULL;
> >  	int single_port_map[1];
> > +	unsigned long index;
> >  
> >  	cxlsd = cxl_switch_decoder_alloc(port, 1);
> >  	if (IS_ERR(cxlsd))
> > @@ -59,7 +60,8 @@ int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
> >  
> >  	device_lock_assert(&port->dev);
> >  
> > -	dport = list_first_entry(&port->dports, typeof(*dport), list);
> > +	xa_for_each(&port->dports, index, dport)
> > +		break;
> >  	single_port_map[0] = dport->port_id;
> >  
> >  	return add_hdm_decoder(port, &cxlsd->cxld, single_port_map);
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index ea3ab9baf232..d2f6898940fa 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -452,6 +452,7 @@ static void cxl_port_release(struct device *dev)
> >  	xa_for_each(&port->endpoints, index, ep)
> >  		cxl_ep_remove(port, ep);
> >  	xa_destroy(&port->endpoints);
> > +	xa_destroy(&port->dports);
> >  	ida_free(&cxl_port_ida, port->id);
> >  	kfree(port);
> >  }
> > @@ -566,7 +567,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
> >  	port->component_reg_phys = component_reg_phys;
> >  	ida_init(&port->decoder_ida);
> >  	port->dpa_end = -1;
> > -	INIT_LIST_HEAD(&port->dports);
> > +	xa_init(&port->dports);
> >  	xa_init(&port->endpoints);
> >  
> >  	device_initialize(dev);
> > @@ -696,17 +697,13 @@ static int match_root_child(struct device *dev, const void *match)
> >  		return 0;
> >  
> >  	port = to_cxl_port(dev);
> > -	device_lock(dev);
> > -	list_for_each_entry(dport, &port->dports, list) {
> > -		iter = match;
> > -		while (iter) {
> > -			if (iter == dport->dport)
> > -				goto out;
> > -			iter = iter->parent;
> > -		}
> > +	iter = match;
> > +	while (iter) {
> > +		dport = cxl_dport_load(port, iter);
> > +		if (dport)
> > +			break;
> > +		iter = iter->parent;
> >  	}
> > -out:
> > -	device_unlock(dev);
> >  
> >  	return !!iter;
> >  }
> > @@ -730,9 +727,10 @@ EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL);
> >  static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> >  {
> >  	struct cxl_dport *dport;
> > +	unsigned long index;
> >  
> >  	device_lock_assert(&port->dev);
> > -	list_for_each_entry (dport, &port->dports, list)
> > +	xa_for_each(&port->dports, index, dport)
> >  		if (dport->port_id == id)
> >  			return dport;
> >  	return NULL;
> > @@ -741,18 +739,21 @@ static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> >  static int add_dport(struct cxl_port *port, struct cxl_dport *new)
> >  {
> >  	struct cxl_dport *dup;
> > +	int rc;
> >  
> >  	device_lock_assert(&port->dev);
> >  	dup = find_dport(port, new->port_id);
> > -	if (dup)
> > +	if (dup) {
> >  		dev_err(&port->dev,
> >  			"unable to add dport%d-%s non-unique port id (%s)\n",
> >  			new->port_id, dev_name(new->dport),
> >  			dev_name(dup->dport));
> > -	else
> > -		list_add_tail(&new->list, &port->dports);
> > +		rc = -EBUSY;
> 
> Direct return slightly simpler and reduce indent on next bit plus makes
> this more obviously an 'error condition' by indenting it.

Looks good, yes.

> 
> > +	} else
> > +		rc = xa_insert(&port->dports, (unsigned long)new->dport, new,
> > +			       GFP_KERNEL);
> >  
> > -	return dup ? -EEXIST : 0;
> > +	return rc;
> >  }
> >  
> >  /*
> > @@ -779,10 +780,8 @@ static void cxl_dport_remove(void *data)
> >  	struct cxl_dport *dport = data;
> >  	struct cxl_port *port = dport->port;
> >  
> > +	xa_erase(&port->dports, (unsigned long) dport->dport);
> >  	put_device(dport->dport);
> > -	cond_cxl_root_lock(port);
> > -	list_del(&dport->list);
> > -	cond_cxl_root_unlock(port);
> >  }
> >  
> >  static void cxl_dport_unlink(void *data)
> > @@ -834,7 +833,6 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> >  	if (!dport)
> >  		return ERR_PTR(-ENOMEM);
> >  
> > -	INIT_LIST_HEAD(&dport->list);
> >  	dport->dport = dport_dev;
> >  	dport->port_id = port_id;
> >  	dport->component_reg_phys = component_reg_phys;
> > @@ -925,7 +923,7 @@ static int match_port_by_dport(struct device *dev, const void *data)
> >  		return 0;
> >  
> >  	port = to_cxl_port(dev);
> > -	dport = cxl_find_dport_by_dev(port, ctx->dport_dev);
> > +	dport = cxl_dport_load(port, ctx->dport_dev);
> >  	if (ctx->dport)
> >  		*ctx->dport = dport;
> >  	return dport != NULL;
> > @@ -1025,19 +1023,27 @@ EXPORT_SYMBOL_NS_GPL(cxl_endpoint_autoremove, CXL);
> >   * for a port to be unregistered is when all memdevs beneath that port have gone
> >   * through ->remove(). This "bottom-up" removal selectively removes individual
> >   * child ports manually. This depends on devm_cxl_add_port() to not change is
> > - * devm action registration order.
> > + * devm action registration order, and for dports to have already been
> > + * destroyed by reap_dports().
> >   */
> > -static void delete_switch_port(struct cxl_port *port, struct list_head *dports)
> > +static void delete_switch_port(struct cxl_port *port)
> > +{
> > +	devm_release_action(port->dev.parent, cxl_unlink_uport, port);
> > +	devm_release_action(port->dev.parent, unregister_port, port);
> > +}
> > +
> > +static void reap_dports(struct cxl_port *port)
> >  {
> > -	struct cxl_dport *dport, *_d;
> > +	struct cxl_dport *dport;
> > +	unsigned long index;
> > +
> > +	device_lock_assert(&port->dev);
> >  
> > -	list_for_each_entry_safe(dport, _d, dports, list) {
> > +	xa_for_each(&port->dports, index, dport) {
> >  		devm_release_action(&port->dev, cxl_dport_unlink, dport);
> >  		devm_release_action(&port->dev, cxl_dport_remove, dport);
> >  		devm_kfree(&port->dev, dport);
> >  	}
> > -	devm_release_action(port->dev.parent, cxl_unlink_uport, port);
> > -	devm_release_action(port->dev.parent, unregister_port, port);
> >  }
> >  
> >  static struct cxl_ep *cxl_ep_load(struct cxl_port *port,
> > @@ -1054,8 +1060,8 @@ static void cxl_detach_ep(void *data)
> >  	for (iter = &cxlmd->dev; iter; iter = grandparent(iter)) {
> >  		struct device *dport_dev = grandparent(iter);
> >  		struct cxl_port *port, *parent_port;
> > -		LIST_HEAD(reap_dports);
> >  		struct cxl_ep *ep;
> > +		bool died = false;
> >  
> >  		if (!dport_dev)
> >  			break;
> > @@ -1095,15 +1101,16 @@ static void cxl_detach_ep(void *data)
> >  			 * enumerated port. Block new cxl_add_ep() and garbage
> >  			 * collect the port.
> >  			 */
> > +			died = true;
> >  			port->dead = true;
> > -			list_splice_init(&port->dports, &reap_dports);
> > +			reap_dports(port);
> 
> I'm not immediately clear on why this refactor is tied up with moving
> to the xarray.  Perhaps a comment in the commit message to add
> more detail around this?

Sure, added the following:

   Note that cxl_detach_ep(), after it determines that the last @ep has
   departed and decides to delete the port, now needs to walk the dport
   array with the device_lock() held to remove entries. Previously
   list_splice_init() could be used atomically delete all dport entries at
   once and then perform entry tear down outside the lock. There is no
   list_splice_init() equivalent for the xarray.