[rdma-next,13/16] RDMA/restrack: Directly mark user/kernel entry in XArray (2024)

From: Leon Romanovsky <leonro@mellanox.com>Conversion to XArray allows us to mark specific entry with custom flags,in our case we need to distinguish between user vs. kernel entries, so dropcustom boolean flag used before in favour of XAarray mark feature.Signed-off-by: Leon Romanovsky <leonro@mellanox.com>--- drivers/infiniband/core/restrack.c | 46 ++++++++++++++++++------------ include/rdma/restrack.h | 9 +----- 2 files changed, 28 insertions(+), 27 deletions(-)

On Mon, Jan 14, 2019 at 04:18:22PM +0200, Leon Romanovsky wrote:> From: Leon Romanovsky <leonro@mellanox.com>> > Conversion to XArray allows us to mark specific entry with custom flags,> in our case we need to distinguish between user vs. kernel entries, so drop> custom boolean flag used before in favour of XAarray mark feature.Marks are mainly helpful in the xarray if they are used as part of theiteration with xa_for_each as they significantly reduce the overhead..> +bool rdma_is_kernel_res(struct rdma_restrack_entry *res)> +{> +struct ib_device *dev = res_to_dev(res);> +struct xarray *xa;> +> +xa = rdma_dev_to_xa(dev, res->type);> +> +return !xa_get_mark(xa, res_to_id(res), RES_USER_ENTRY);> +}> +EXPORT_SYMBOL(rdma_is_kernel_res);This is now O(log(n)) to save an irrelevant amount of memory.Jason

On Mon, Jan 14, 2019 at 08:37:55PM +0000, Jason Gunthorpe wrote:> On Mon, Jan 14, 2019 at 04:18:22PM +0200, Leon Romanovsky wrote:> > From: Leon Romanovsky <leonro@mellanox.com>> >> > Conversion to XArray allows us to mark specific entry with custom flags,> > in our case we need to distinguish between user vs. kernel entries, so drop> > custom boolean flag used before in favour of XAarray mark feature.>> Marks are mainly helpful in the xarray if they are used as part of the> iteration with xa_for_each as they significantly reduce the overhead..>> > +bool rdma_is_kernel_res(struct rdma_restrack_entry *res)> > +{> > +struct ib_device *dev = res_to_dev(res);> > +struct xarray *xa;> > +> > +xa = rdma_dev_to_xa(dev, res->type);> > +> > +return !xa_get_mark(xa, res_to_id(res), RES_USER_ENTRY);> > +}> > +EXPORT_SYMBOL(rdma_is_kernel_res);>> This is now O(log(n)) to save an irrelevant amount of memory.Yes, now it takes 3 bits which anyway were allocated as part of xa_statehandling. Before we had O(k) in memory footprint, while "k" is number of allobjects and "n" is number of valid objects only.Thanks>> Jason

On Tue, Jan 15, 2019 at 02:25:11PM +0200, Leon Romanovsky wrote:> On Mon, Jan 14, 2019 at 08:37:55PM +0000, Jason Gunthorpe wrote:> > On Mon, Jan 14, 2019 at 04:18:22PM +0200, Leon Romanovsky wrote:> > > From: Leon Romanovsky <leonro@mellanox.com>> > >> > > Conversion to XArray allows us to mark specific entry with custom flags,> > > in our case we need to distinguish between user vs. kernel entries, so drop> > > custom boolean flag used before in favour of XAarray mark feature.> >> > Marks are mainly helpful in the xarray if they are used as part of the> > iteration with xa_for_each as they significantly reduce the overhead..> >> > > +bool rdma_is_kernel_res(struct rdma_restrack_entry *res)> > > +{> > > +struct ib_device *dev = res_to_dev(res);> > > +struct xarray *xa;> > > +> > > +xa = rdma_dev_to_xa(dev, res->type);> > > +> > > +return !xa_get_mark(xa, res_to_id(res), RES_USER_ENTRY);> > > +}> > > +EXPORT_SYMBOL(rdma_is_kernel_res);> >> > This is now O(log(n)) to save an irrelevant amount of memory.> > Yes, now it takes 3 bits which anyway were allocated as part of xa_state> handling. Before we had O(k) in memory footprint, while "k" is number of all> objects and "n" is number of valid objects only.If memory is the issue here the optimize the packing of the restrackentry, I don't see a good reason to use marks for this..Jason

On Tue, Jan 15, 2019 at 09:54:00AM -0700, Jason Gunthorpe wrote:> On Tue, Jan 15, 2019 at 02:25:11PM +0200, Leon Romanovsky wrote:> > On Mon, Jan 14, 2019 at 08:37:55PM +0000, Jason Gunthorpe wrote:> > > On Mon, Jan 14, 2019 at 04:18:22PM +0200, Leon Romanovsky wrote:> > > > From: Leon Romanovsky <leonro@mellanox.com>> > > >> > > > Conversion to XArray allows us to mark specific entry with custom flags,> > > > in our case we need to distinguish between user vs. kernel entries, so drop> > > > custom boolean flag used before in favour of XAarray mark feature.> > >> > > Marks are mainly helpful in the xarray if they are used as part of the> > > iteration with xa_for_each as they significantly reduce the overhead..> > >> > > > +bool rdma_is_kernel_res(struct rdma_restrack_entry *res)> > > > +{> > > > +struct ib_device *dev = res_to_dev(res);> > > > +struct xarray *xa;> > > > +> > > > +xa = rdma_dev_to_xa(dev, res->type);> > > > +> > > > +return !xa_get_mark(xa, res_to_id(res), RES_USER_ENTRY);> > > > +}> > > > +EXPORT_SYMBOL(rdma_is_kernel_res);> > >> > > This is now O(log(n)) to save an irrelevant amount of memory.> >> > Yes, now it takes 3 bits which anyway were allocated as part of xa_state> > handling. Before we had O(k) in memory footprint, while "k" is number of all> > objects and "n" is number of valid objects only.>> If memory is the issue here the optimize the packing of the restrack> entry, I don't see a good reason to use marks for this..I see using marks as more convenient API, instead of using some bool value.My intention is to remove duplicated fields and "user" is one of them.Thanks>> Jason

Patch

diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.cindex 5df8e4607f09..c50e95a82915 100644--- a/drivers/infiniband/core/restrack.c+++ b/drivers/infiniband/core/restrack.c@@ -232,42 +232,29 @@  static void rdma_restrack_add(struct rdma_restrack_entry *res) { struct ib_device *dev = res_to_dev(res); struct xarray *xa = rdma_dev_to_xa(dev, res->type);-unsigned long id; int ret; -if (!dev)-return;--if (res->type != RDMA_RESTRACK_CM_ID || rdma_is_kernel_res(res))-res->task = NULL;--if (!rdma_is_kernel_res(res)) {-if (!res->task)-rdma_restrack_set_task(res, NULL);-res->kern_name = NULL;-} else {-set_kern_name(res);-}- kref_init(&res->kref); init_completion(&res->comp); res->valid = true; -id = res_to_id(res);-ret = xa_insert(xa, id, res, GFP_KERNEL);+ret = xa_insert(xa, res_to_id(res), res, GFP_KERNEL); WARN_ONCE(ret == -EEXIST, "Tried to add non-unique type %d entry\n", res->type); if (ret) res->valid = false; } +#define RES_USER_ENTRYXA_MARK_1+ /** * rdma_restrack_kadd() - add kernel object to the reource tracking database * @res: resource entry */ void rdma_restrack_kadd(struct rdma_restrack_entry *res) {-res->user = false;+res->task = NULL;+set_kern_name(res); rdma_restrack_add(res); } EXPORT_SYMBOL(rdma_restrack_kadd);@@ -278,11 +265,32 @@  EXPORT_SYMBOL(rdma_restrack_kadd); */ void rdma_restrack_uadd(struct rdma_restrack_entry *res) {-res->user = true;+struct ib_device *dev = res_to_dev(res);+struct xarray *xa = rdma_dev_to_xa(dev, res->type);++if (res->type != RDMA_RESTRACK_CM_ID)+res->task = NULL;++if (!res->task)+rdma_restrack_set_task(res, NULL);+res->kern_name = NULL;+ rdma_restrack_add(res);+xa_set_mark(xa, res_to_id(res), RES_USER_ENTRY); } EXPORT_SYMBOL(rdma_restrack_uadd); +bool rdma_is_kernel_res(struct rdma_restrack_entry *res)+{+struct ib_device *dev = res_to_dev(res);+struct xarray *xa;++xa = rdma_dev_to_xa(dev, res->type);++return !xa_get_mark(xa, res_to_id(res), RES_USER_ENTRY);+}+EXPORT_SYMBOL(rdma_is_kernel_res);+ int __must_check rdma_restrack_get(struct rdma_restrack_entry *res) { return kref_get_unless_zero(&res->kref);diff --git a/include/rdma/restrack.h b/include/rdma/restrack.hindex dbbce1b29b12..09cc4eff3f9d 100644--- a/include/rdma/restrack.h+++ b/include/rdma/restrack.h@@ -89,10 +89,6 @@  struct rdma_restrack_entry { * @type: various objects in restrack database */ enum rdma_restrack_typetype;-/**- * @user: user resource- */-booluser; }; /**@@ -133,10 +129,7 @@  void rdma_restrack_del(struct rdma_restrack_entry *res); * rdma_is_kernel_res() - check the owner of resource * @res: resource entry */-static inline bool rdma_is_kernel_res(struct rdma_restrack_entry *res)-{-return !res->user;-}+bool rdma_is_kernel_res(struct rdma_restrack_entry *res); /** * rdma_restrack_get() - grab to protect resource from release