brd in a memdesc world

brd in a memdesc world

[Matthew Wilcox]

Hi Jens,

I'm looking for an architecture-level decision on what the brd driver
should look like once struct page has been shrunk to a minimal size
(more detail at https://kernelnewbies.org/MatthewWilcox/Memdescs )

Currently brd uses page->index as a debugging check.  In the memdesc
future, struct page has no members (you could store a small amount of
information in it, but I'm not willing to commit to more than a few bits).

brd doesn't use anything else from struct page, as far as I can tell.
It just calls kmap_atomic() / __free_page() / flush_dcache_page() (and
it doesn't need to call flush_dcache_page() because you can't mmap the
pages in the brd's array).

Now if you have plans to, eg, support page migration, you're going to need
a bit more infrastructure than just allocating pages, but for what you
have at the moment, just removing the debugging checks that page->index ==
idx would make you entirely compatible with the memdesc future.

Any problem with that?

Re: brd in a memdesc world

[Pankaj Raghav]

On 12/03/2024 18:40, Matthew Wilcox wrote:
> Hi Jens,
> 
> I'm looking for an architecture-level decision on what the brd driver
> should look like once struct page has been shrunk to a minimal size
> (more detail at https://protect2.fireeye.com/v1/url?k=fdf5d9a0-9c7ecc9a-fdf452ef-74fe4860008a-d5306bf365c2b9b6&q=1&e=cbceae8b-61fb-4e3e-8f7c-6717d9b2431d&u=https%3A%2F%2Fkernelnewbies.org%2FMatthewWilcox%2FMemdescs )
> 
> Currently brd uses page->index as a debugging check.  In the memdesc
> future, struct page has no members (you could store a small amount of
> information in it, but I'm not willing to commit to more than a few bits).
> 

Shouldn't we change brd to use folios? Once we do that, this will not
be a problem any more right?

Hannes even had patches around it long time back [1]

> brd doesn't use anything else from struct page, as far as I can tell.
> It just calls kmap_atomic() / __free_page() / flush_dcache_page() (and
> it doesn't need to call flush_dcache_page() because you can't mmap the
> pages in the brd's array).
> 
> Now if you have plans to, eg, support page migration, you're going to need
> a bit more infrastructure than just allocating pages, but for what you
> have at the moment, just removing the debugging checks that page->index ==
> idx would make you entirely compatible with the memdesc future.
> 
> Any problem with that?

[1] https://lore.kernel.org/linux-block/20230306120127.21375-2-hare@suse.de/

Re: brd in a memdesc world

[Matthew Wilcox]

On Wed, Mar 13, 2024 at 06:15:26PM +0100, Pankaj Raghav wrote:
> On 12/03/2024 18:40, Matthew Wilcox wrote:
> > Hi Jens,
> > 
> > I'm looking for an architecture-level decision on what the brd driver
> > should look like once struct page has been shrunk to a minimal size
> > (more detail at https://protect2.fireeye.com/v1/url?k=fdf5d9a0-9c7ecc9a-fdf452ef-74fe4860008a-d5306bf365c2b9b6&q=1&e=cbceae8b-61fb-4e3e-8f7c-6717d9b2431d&u=https%3A%2F%2Fkernelnewbies.org%2FMatthewWilcox%2FMemdescs )
> > 
> > Currently brd uses page->index as a debugging check.  In the memdesc
> > future, struct page has no members (you could store a small amount of
> > information in it, but I'm not willing to commit to more than a few bits).
> > 
> 
> Shouldn't we change brd to use folios? Once we do that, this will not
> be a problem any more right?

We certainly could change brd to use folios.  But why would we want to?
Hannes' work always allocates memory of a fixed size (a fixed multiple
of PAGE_SIZE).  Folios are a medium-weight data structure (probably
about 80 bytes once we get to memdescs).  They support a lot of things,
eg belonging to an inode, having an index, being mappable to userspace,
being lockable, accountable to memcgs, allowing extra private data,
knowing their own size, ...

None of those things are needed for brd's uses.  All brd needs is to
be able to allocate, kmap and free chunks of memory.  Unless there are
plans to do more than this.

Re: brd in a memdesc world

[Pankaj Raghav]

>>> Currently brd uses page->index as a debugging check.  In the memdesc
>>> future, struct page has no members (you could store a small amount of
>>> information in it, but I'm not willing to commit to more than a few bits).
>>>
>>
>> Shouldn't we change brd to use folios? Once we do that, this will not
>> be a problem any more right?
> 
> We certainly could change brd to use folios.  But why would we want to?
> Hannes' work always allocates memory of a fixed size (a fixed multiple
> of PAGE_SIZE).  Folios are a medium-weight data structure (probably
> about 80 bytes once we get to memdescs).  They support a lot of things,
> eg belonging to an inode, having an index, being mappable to userspace,
> being lockable, accountable to memcgs, allowing extra private data,
> knowing their own size, ...
> 

Got it! Probably moving to folios just for the sake of retaining the
debugging checks is not enough.

> None of those things are needed for brd's uses.  All brd needs is to
> be able to allocate, kmap and free chunks of memory.  Unless there are
> plans to do more than this.
> 

I remember he mentioned he wanted to support bigger logical block sizes
in brd, in which case moving to folios might be justified.

Re: brd in a memdesc world

[Matthew Wilcox]

On Wed, Mar 13, 2024 at 07:36:01PM +0100, Pankaj Raghav wrote:
> Got it! Probably moving to folios just for the sake of retaining the
> debugging checks is not enough.

That would be my assessment, but maybe Jens has a different preference.

> > None of those things are needed for brd's uses.  All brd needs is to
> > be able to allocate, kmap and free chunks of memory.  Unless there are
> > plans to do more than this.
> 
> I remember he mentioned he wanted to support bigger logical block sizes
> in brd, in which case moving to folios might be justified.

Using folios (or some other data structure that embedded the order of
the allocation) would be justified if there was a proposal on the table
to support variable sized allocations in the brd XArray.  Hannes wasn't
suggesting that; his patch series used a fixed size per block device
(which honestly makes sense; I don't see the advantage to supporting
variable sized allocations).

Re: brd in a memdesc world

[Hannes Reinecke]

On 3/13/24 19:28, Matthew Wilcox wrote:
> On Wed, Mar 13, 2024 at 06:15:26PM +0100, Pankaj Raghav wrote:
>> On 12/03/2024 18:40, Matthew Wilcox wrote:
>>> Hi Jens,
>>>
>>> I'm looking for an architecture-level decision on what the brd driver
>>> should look like once struct page has been shrunk to a minimal size
>>> (more detail at https://protect2.fireeye.com/v1/url?k=fdf5d9a0-9c7ecc9a-fdf452ef-74fe4860008a-d5306bf365c2b9b6&q=1&e=cbceae8b-61fb-4e3e-8f7c-6717d9b2431d&u=https%3A%2F%2Fkernelnewbies.org%2FMatthewWilcox%2FMemdescs )
>>>
>>> Currently brd uses page->index as a debugging check.  In the memdesc
>>> future, struct page has no members (you could store a small amount of
>>> information in it, but I'm not willing to commit to more than a few bits).
>>>
>>
>> Shouldn't we change brd to use folios? Once we do that, this will not
>> be a problem any more right?
> 
> We certainly could change brd to use folios.  But why would we want to?
> Hannes' work always allocates memory of a fixed size (a fixed multiple
> of PAGE_SIZE).  Folios are a medium-weight data structure (probably
> about 80 bytes once we get to memdescs).  They support a lot of things,
> eg belonging to an inode, having an index, being mappable to userspace,
> being lockable, accountable to memcgs, allowing extra private data,
> knowing their own size, ...
> 
> None of those things are needed for brd's uses.  All brd needs is to
> be able to allocate, kmap and free chunks of memory.  Unless there are
> plans to do more than this.
> 
The primary goal of my patchset is to make brd a test-bed for the LBS 
work; devices with a sector size larger than 4k are really hard to come
by. But really, it's just a testbed, and I'm not sure whether there's
a need for that in the general audience.
I can resubmit if you want ...

As for the memory overhead, I guess it only makes a noticable difference
when moving to hugepages, and have brd allocate hugepages only.
But that is future work for sure.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich

Re: brd in a memdesc world

[Matthew Wilcox]

On Thu, Mar 14, 2024 at 12:11:18PM +0100, Hannes Reinecke wrote:
> On 3/13/24 19:28, Matthew Wilcox wrote:
> > None of those things are needed for brd's uses.  All brd needs is to
> > be able to allocate, kmap and free chunks of memory.  Unless there are
> > plans to do more than this.
> 
> The primary goal of my patchset is to make brd a test-bed for the LBS work;
> devices with a sector size larger than 4k are really hard to come
> by. But really, it's just a testbed, and I'm not sure whether there's
> a need for that in the general audience.
> I can resubmit if you want ...
> 
> As for the memory overhead, I guess it only makes a noticable difference
> when moving to hugepages, and have brd allocate hugepages only.
> But that is future work for sure.

As I said, this is talking about a memdesc future where struct page is a
mere 8 bytes and most of its contents are reserved to the MM.  At this
point, there's no extra overhead to your patch, but there would be when
we get there.

So I don't think the folio conversion is necessary, but I think
supporting larger sector sizes in brd is a worthwhile extension.

Re: brd in a memdesc world

[Jens Axboe]

On 3/12/24 11:40 AM, Matthew Wilcox wrote:
> Hi Jens,
> 
> I'm looking for an architecture-level decision on what the brd driver
> should look like once struct page has been shrunk to a minimal size
> (more detail at https://kernelnewbies.org/MatthewWilcox/Memdescs )
> 
> Currently brd uses page->index as a debugging check.  In the memdesc
> future, struct page has no members (you could store a small amount of
> information in it, but I'm not willing to commit to more than a few bits).
> 
> brd doesn't use anything else from struct page, as far as I can tell.
> It just calls kmap_atomic() / __free_page() / flush_dcache_page() (and
> it doesn't need to call flush_dcache_page() because you can't mmap the
> pages in the brd's array).
> 
> Now if you have plans to, eg, support page migration, you're going to need
> a bit more infrastructure than just allocating pages, but for what you
> have at the moment, just removing the debugging checks that page->index ==
> idx would make you entirely compatible with the memdesc future.
> 
> Any problem with that?

As far as I can recall, I haven't seen that debugging trigger anything
in forever. So fine to kill it, imho.

-- 
Jens Axboe