LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Alex Elder <elder@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Johan Hovold <johan@kernel.org>,
	greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray
Date: Tue, 17 Aug 2021 12:17:13 +0200	[thread overview]
Message-ID: <2337511.Rlusi4go5H@localhost.localdomain> (raw)
In-Reply-To: <5541b638-db1e-26f2-2682-81f35504c9a3@ieee.org>

On Monday, August 16, 2021 4:46:08 PM CEST Alex Elder wrote:
> On 8/14/21 1:11 PM, Fabio M. De Francesco wrote:
> > Convert greybus/uart.c from IDR to XArray. The abstract data type XArray
> > is more memory-efficient, parallelisable, and cache friendly. It takes
> > advantage of RCU to perform lookups without locking. Furthermore, IDR is
> > deprecated because XArray has a better (cleaner and more consistent) API.
> 
> I haven't verified the use of the new API (yet) but I have a few
> comments on your patch, below.
> 
> 					-Alex
> 
Hi Alex,

As I promised in another message, I've already submitted a v3 of this patch:
https://lkml.org/lkml/2021/8/16/1188

While I'm pretty sure that XArray should be used in place of the older and less 
efficient IDR (some time ago Matthew Wilcox agreed and confirmed that this
is true), I'm not entirely sure if we should also prefer XArray over IDA for this 
particular driver.

Initially I had decided to convert the other greybus file from IDA to XArray but
then I stopped because of the above-mentioned doubts. 

I really don't know if it is worth doing this work. As far as I understand these API,
IDA (although it is not as versatile as IDR is) is more  memory efficient than IDR.
In documentation I read: "The IDA is an ID allocator which does not provide the 
ability to associate an ID with a pointer. As such, it only needs to store one bit 
per ID, and so is more space efficient than an IDR.".

May you please say if you think that the driver would also benefit by the 
conversion from IDA to XArray?

Thanks,

Fabio




  parent reply	other threads:[~2021-08-17 10:17 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-14 18:11 Fabio M. De Francesco
2021-08-16 14:46 ` Alex Elder
2021-08-16 15:01   ` Greg Kroah-Hartman
2021-08-16 15:06     ` Dan Carpenter
2021-08-16 15:10       ` [greybus-dev] " Alex Elder
2021-08-16 18:36         ` Dan Carpenter
2021-08-16 18:38           ` Dan Carpenter
2021-08-16 19:17           ` Alex Elder
2021-08-17  4:58             ` Dan Carpenter
2021-08-16 16:55   ` Fabio M. De Francesco
2021-08-17 10:17   ` Fabio M. De Francesco [this message]
2021-08-25  5:20   ` Fabio M. De Francesco
2021-08-25 13:45     ` [greybus-dev] " Alex Elder
2021-08-26 15:57       ` Fabio M. De Francesco

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2337511.Rlusi4go5H@localhost.localdomain \
    --to=fmdefrancesco@gmail.com \
    --cc=elder@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=greybus-dev@lists.linaro.org \
    --cc=johan@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --subject='Re: [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).