All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yury Norov <yury.norov@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
	agk@redhat.com, snitzer@kernel.org, dm-devel@redhat.com,
	vneethv@linux.ibm.com, oberpar@linux.ibm.com, hca@linux.ibm.com,
	gor@linux.ibm.com, agordeev@linux.ibm.com,
	borntraeger@linux.ibm.com, svens@linux.ibm.com,
	almaz.alexandrovich@paragon-software.com,
	linux@rasmusvillemoes.dk, linux-s390@vger.kernel.org,
	ntfs3@lists.linux.dev, linux-kernel@vger.kernel.org,
	kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 3/4] bitmap: Introduce bitmap_size()
Date: Sun, 3 Jul 2022 12:13:08 -0700	[thread overview]
Message-ID: <YsHqRDfCQVwyA2m1@yury-laptop> (raw)
In-Reply-To: <YsGz1Xp0RDM5ZhVY@smile.fi.intel.com>

On Sun, Jul 03, 2022 at 06:20:53PM +0300, Andy Shevchenko wrote:
> On Sun, Jul 03, 2022 at 08:50:19AM +0200, Christophe JAILLET wrote:
> > Le 02/07/2022 à 23:09, Yury Norov a écrit :
> > > On Sat, Jul 02, 2022 at 08:29:36PM +0200, Christophe JAILLET wrote:
> 
> ...
> 
> > > This should be dropped, for sure, and kmalloc() at line 128 should be
> > > replaced with bitmap_alloc().
> > 
> > This kmalloc() is for a structure and a flexible array.
> > 
> > You mean re-arranging the code to allocate the structure alone at first,
> > then the bitmap?

We can change struct primes to:
        struct primes {
                struct rcu_head rcu;
                unsigned long last, sz;
                unsigned long *primes;
        };

And then either allocate twice:
        new = kmalloc(sizeof(struct primes);
        new->primes = bitmap_alloc(sz);

Or keep the same struct primes for all expansions, and just allocate
new bitmap for ->primes when needed. This is what I meant.

This a bit deeper rework, but it addresses Andy's concern about excessive
fragmentation. (Did anyone before complain? Is it measurable?)

> It's one way, but it will increase fragmentation of memory. The other one
> as it seems to me is to name a new API properly, i.e. bitmap_size_to_bytes().
>
> In such case you won't need renames to begin with. And then would be able
> to convert driver-by-driver in cases of duplicated code.
> 
> I think that's what confused Yuri and I kinda agree that bitmap_size() should
> return bits, and not bytes. Also argument for pure bitmap_size() would be
> bitmap itself, but we have no way to detect the length of bitmap because we
> are using POD and not a specific data structure for it.

bitmap_size_to_bytes() sounds better. How many places in the kernel
do we have where we can't simply use bitmap_alloc(), and need this
machinery? If this is the only one, I'd prefer to switch it to
bitmap_alloc() instead.

Thanks,
Yury

WARNING: multiple messages have this Message-ID (diff)
From: Yury Norov <yury.norov@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-s390@vger.kernel.org, kernel-janitors@vger.kernel.org,
	ntfs3@lists.linux.dev, gor@linux.ibm.com,
	linux@rasmusvillemoes.dk, hca@linux.ibm.com, snitzer@kernel.org,
	oberpar@linux.ibm.com, linux-kernel@vger.kernel.org,
	almaz.alexandrovich@paragon-software.com, dm-devel@redhat.com,
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
	svens@linux.ibm.com, vneethv@linux.ibm.com,
	agordeev@linux.ibm.com, borntraeger@linux.ibm.com,
	agk@redhat.com
Subject: Re: [dm-devel] [PATCH 3/4] bitmap: Introduce bitmap_size()
Date: Sun, 3 Jul 2022 12:13:08 -0700	[thread overview]
Message-ID: <YsHqRDfCQVwyA2m1@yury-laptop> (raw)
In-Reply-To: <YsGz1Xp0RDM5ZhVY@smile.fi.intel.com>

On Sun, Jul 03, 2022 at 06:20:53PM +0300, Andy Shevchenko wrote:
> On Sun, Jul 03, 2022 at 08:50:19AM +0200, Christophe JAILLET wrote:
> > Le 02/07/2022 à 23:09, Yury Norov a écrit :
> > > On Sat, Jul 02, 2022 at 08:29:36PM +0200, Christophe JAILLET wrote:
> 
> ...
> 
> > > This should be dropped, for sure, and kmalloc() at line 128 should be
> > > replaced with bitmap_alloc().
> > 
> > This kmalloc() is for a structure and a flexible array.
> > 
> > You mean re-arranging the code to allocate the structure alone at first,
> > then the bitmap?

We can change struct primes to:
        struct primes {
                struct rcu_head rcu;
                unsigned long last, sz;
                unsigned long *primes;
        };

And then either allocate twice:
        new = kmalloc(sizeof(struct primes);
        new->primes = bitmap_alloc(sz);

Or keep the same struct primes for all expansions, and just allocate
new bitmap for ->primes when needed. This is what I meant.

This a bit deeper rework, but it addresses Andy's concern about excessive
fragmentation. (Did anyone before complain? Is it measurable?)

> It's one way, but it will increase fragmentation of memory. The other one
> as it seems to me is to name a new API properly, i.e. bitmap_size_to_bytes().
>
> In such case you won't need renames to begin with. And then would be able
> to convert driver-by-driver in cases of duplicated code.
> 
> I think that's what confused Yuri and I kinda agree that bitmap_size() should
> return bits, and not bytes. Also argument for pure bitmap_size() would be
> bitmap itself, but we have no way to detect the length of bitmap because we
> are using POD and not a specific data structure for it.

bitmap_size_to_bytes() sounds better. How many places in the kernel
do we have where we can't simply use bitmap_alloc(), and need this
machinery? If this is the only one, I'd prefer to switch it to
bitmap_alloc() instead.

Thanks,
Yury

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


  reply	other threads:[~2022-07-03 19:13 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-02 18:28 [PATCH 0/4] Introduce bitmap_size() Christophe JAILLET
2022-07-02 18:28 ` [dm-devel] " Christophe JAILLET
2022-07-02 18:29 ` [PATCH 1/4] s390/cio: Rename bitmap_size() as idset_bitmap_size() Christophe JAILLET
2022-07-02 18:29   ` [dm-devel] " Christophe JAILLET
2022-07-02 18:54   ` Andy Shevchenko
2022-07-02 18:54     ` [dm-devel] " Andy Shevchenko
2022-07-02 19:24     ` Christophe JAILLET
2022-07-02 19:24       ` [dm-devel] " Christophe JAILLET
2022-07-02 19:24       ` Christophe JAILLET
2022-07-02 19:32       ` Andy Shevchenko
2022-07-02 19:32         ` [dm-devel] " Andy Shevchenko
2022-07-02 19:42         ` Christophe JAILLET
2022-07-02 19:42           ` [dm-devel] " Christophe JAILLET
2022-07-02 19:42           ` Christophe JAILLET
2022-07-02 19:42           ` Christophe JAILLET
2022-07-02 20:46   ` Yury Norov
2022-07-02 20:46     ` [dm-devel] " Yury Norov
2022-07-04  4:28   ` Vineeth Vijayan
2022-07-04  4:28     ` [dm-devel] " Vineeth Vijayan
2022-07-02 18:29 ` [PATCH 2/4] fs/ntfs3: Rename bitmap_size() as ntfs3_bitmap_size() Christophe JAILLET
2022-07-02 18:29   ` [dm-devel] " Christophe JAILLET
2022-07-02 18:58   ` Andy Shevchenko
2022-07-02 18:58     ` [dm-devel] " Andy Shevchenko
2022-07-02 19:37     ` Christophe JAILLET
2022-07-02 19:37       ` [dm-devel] " Christophe JAILLET
2022-07-02 19:37       ` Christophe JAILLET
2022-07-02 20:57   ` Yury Norov
2022-07-02 20:57     ` [dm-devel] " Yury Norov
2022-07-02 18:29 ` [PATCH 3/4] bitmap: Introduce bitmap_size() Christophe JAILLET
2022-07-02 18:29   ` [dm-devel] " Christophe JAILLET
2022-07-02 18:59   ` Andy Shevchenko
2022-07-02 18:59     ` [dm-devel] " Andy Shevchenko
2022-07-02 21:09   ` Yury Norov
2022-07-02 21:09     ` [dm-devel] " Yury Norov
2022-07-03  6:50     ` Christophe JAILLET
2022-07-03  6:50       ` [dm-devel] " Christophe JAILLET
2022-07-03  6:50       ` Christophe JAILLET
2022-07-03 15:20       ` Andy Shevchenko
2022-07-03 15:20         ` [dm-devel] " Andy Shevchenko
2022-07-03 19:13         ` Yury Norov [this message]
2022-07-03 19:13           ` Yury Norov
2022-07-03 20:41           ` Christophe JAILLET
2022-07-03 20:41             ` [dm-devel] " Christophe JAILLET
2022-07-02 18:29 ` [PATCH 4/4] bitmap: Use bitmap_size() Christophe JAILLET
2022-07-02 18:29   ` [dm-devel] " Christophe JAILLET
2022-07-02 20:44 ` [PATCH 0/4] Introduce bitmap_size() Yury Norov
2022-07-02 20:44   ` [dm-devel] " Yury Norov

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=YsHqRDfCQVwyA2m1@yury-laptop \
    --to=yury.norov@gmail.com \
    --cc=agk@redhat.com \
    --cc=agordeev@linux.ibm.com \
    --cc=almaz.alexandrovich@paragon-software.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=borntraeger@linux.ibm.com \
    --cc=christophe.jaillet@wanadoo.fr \
    --cc=dm-devel@redhat.com \
    --cc=gor@linux.ibm.com \
    --cc=hca@linux.ibm.com \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=ntfs3@lists.linux.dev \
    --cc=oberpar@linux.ibm.com \
    --cc=snitzer@kernel.org \
    --cc=svens@linux.ibm.com \
    --cc=vneethv@linux.ibm.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.