On Tue, Jan 29, 2019 at 08:34:37AM +0200, Leon Romanovsky wrote: > On Mon, Jan 28, 2019 at 10:23:33PM -0700, Jason Gunthorpe wrote: > > On Tue, Jan 22, 2019 at 08:15:48PM +0200, Leon Romanovsky wrote: > > > > > + /* > > > + * Once all drivers are converted, we can remove this check > > > + * and remove call to rdma_restrack_add() from rdma_restrack_kadd() > > > + * and rdma_restrack_uadd() > > > + */ > > > + if (xa_load(xa, res->id)) > > > + return 0; > > > > So we check if the ID is present.. Because the driver did something? > > But no drivers do in this series, I guess? Don't get it. > > This patch annotated driver with rdma_restrcak_add() > https://patchwork.kernel.org/patch/10782985/ I don't think you should try and have a complicated 3 state lifetime model. If the pointer is in the xarray then it should be a fully live object ready for all users. Otherwise it shouldn't be in the xarray at all. Adding half initialized objects is a good way to create very subtle bugs, and trying to guess the status of the lifetime model by doing an xarray search seems pretty wild. > It makes some paths with double calls to rdma_restrcak_add(), which this > cxa_load() prevents. I don't think we should do that at all. If drivers need to use the shared ID alloctor they should do a restrack_reserve_id(&ibpd->restrack, ...); And not try and resequence 'add' Otherwise the core code should simply allocate them an ID and drivers shouldn't be changed. > > > @@ -393,8 +469,7 @@ void rdma_restrack_del(struct rdma_restrack_entry *res) > > > return; > > > > > > xa = rdma_dev_to_xa(dev, res->type); > > > - id = res_to_id(res); > > > - if (!xa_load(xa, id)) > > > + if (!xa_load(xa, res->id)) > > > goto out; > > > > Seems strange, isn't this sort of test what valid is supposed to be > > used for? Why do we have valid, !NULL in the xarray and RES_VISIBLE > > all at once? > > I need a way to distinguish between restrack entries without ib_device > set and CM_IDs, see the comment above "if (!dev)" check. It is confusing, how can an restrack with !dev even be considered valid? > > This sounds like it is re-coding xa_reserve? Why can't the driver use > > xa_reserve to hold its ID and we can just make the xarray value > > non-NULL when it the entry is ready to go? Seems simpler if the driver > > needs to rely on the retrack allocator for IDs.. > > Not really, I see restrack as a perfect id-to-object translator and > not only allocator for IDs. It will allow to remove endless number > of home-grown radix trees in drivers. This id-to-object translator > requires that object != NULL for relevant ID. Do drivers really need to have their pointers in the radix tree before the core code has completed initializing the object? That seems pretty hard to believe / will create bugs.. Is there some example you are looking at for this? This needs a much better commenting to understand that what the lifetime model here is supposed to be, and how drivers should use it. Jason
