Hi,

> -----Original Message-----
> From: Hannes Frederic Sowa [mailto:han...@stressinduktion.org]
> Sent: Tuesday, August 29, 2017 3:14 PM
> To: Chris Mi <chr...@mellanox.com>
> Cc: netdev@vger.kernel.org; j...@mojatatu.com;
> xiyou.wangc...@gmail.com; j...@resnulli.us; da...@davemloft.net;
> mawil...@microsoft.com
> Subject: Re: [patch net-next 1/3] idr: Add new APIs to support unsigned long
> 
> Hello,
> 
> Chris Mi <chr...@mellanox.com> writes:
> 
> > 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 <chr...@mellanox.com>
> > Signed-off-by: Jiri Pirko <j...@mellanox.com>
> > ---
> >  include/linux/idr.h        | 16 ++++++++++
> >  include/linux/radix-tree.h |  3 ++
> >  lib/idr.c                  | 56 +++++++++++++++++++++++++++++++++++
> >  lib/radix-tree.c           | 73
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 148 insertions(+)
> >
> 
> [...]
> 
> > +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

Reply via email to