RE: [patch net-next 1/3] idr: Add new APIs to support unsigned long

Hi,

-----Original Message-----
From: Hannes Frederic Sowa [mailto:hannes@stressinduktion.org]
Sent: Tuesday, August 29, 2017 3:14 PM
To: Chris Mi <redacted>
Cc: netdev@vger.kernel.org; jhs@mojatatu.com;
xiyou.wangcong@gmail.com; jiri@resnulli.us; davem@davemloft.net;
mawilcox@microsoft.com
Subject: Re: [patch net-next 1/3] idr: Add new APIs to support unsigned long

Hello,

Chris Mi [off-list ref] writes:

quoted

The following new APIs are added:

int idr_alloc_ext(struct idr *idr, void *ptr, unsigned long *index,
                  unsigned long start, unsigned long end, gfp_t gfp);
static inline void *idr_remove_ext(struct idr *idr, unsigned long id);
static inline void *idr_find_ext(const struct idr *idr, unsigned long
id); void *idr_replace_ext(struct idr *idr, void *ptr, unsigned long
id); void *idr_get_next_ext(struct idr *idr, unsigned long *nextid);

Signed-off-by: Chris Mi <redacted>
Signed-off-by: Jiri Pirko <redacted>
---
 include/linux/idr.h        | 16 ++++++++++
 include/linux/radix-tree.h |  3 ++
 lib/idr.c                  | 56 +++++++++++++++++++++++++++++++++++
 lib/radix-tree.c           | 73

++++++++++++++++++++++++++++++++++++++++++++++

quoted

 4 files changed, 148 insertions(+)

[...]

quoted

+int idr_alloc_ext(struct idr *idr, void *ptr, unsigned long *index,
+		  unsigned long start, unsigned long end, gfp_t gfp) {
+	void __rcu **slot;
+	struct radix_tree_iter iter;
+
+	if (WARN_ON_ONCE(radix_tree_is_internal_node(ptr)))
+		return -EINVAL;
+
+	radix_tree_iter_init(&iter, start);
+	slot = idr_get_free_ext(&idr->idr_rt, &iter, gfp, end);
+	if (IS_ERR(slot))
+		return PTR_ERR(slot);
+
+	radix_tree_iter_replace(&idr->idr_rt, &iter, slot, ptr);
+	radix_tree_iter_tag_clear(&idr->idr_rt, &iter, IDR_FREE);
+
+	if (index)
+		*index = iter.index;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(idr_alloc_ext);

Can you express idr_alloc in terms of idr_alloc_ext? Same for most of the
other functions (it seems that signed int was used as return value to indicate
error cases, thus it should be easy to map those).

In idr_alloc(), we have the following check:

        if (WARN_ON_ONCE(start < 0))
                return -EINVAL;

But in idr_alloc_ext(), since we are using unsigned long, we needn't such check.

In order to reuse several lines of code, I think it is not worth to express idr_alloc()
In terms of idr_alloc_ext. 

Thanks,
Chris

[...]

Thanks,
Hannes