Re: [PATCH v5 4/6] mm/slab: Introduce kmem_buckets_create() and family

Re: [PATCH v5 4/6] mm/slab: Introduce kmem_buckets_create() and family

[Vlastimil Babka]

On 6/20/24 8:54 PM, Kees Cook wrote:
> On Thu, Jun 20, 2024 at 03:56:27PM +0200, Vlastimil Babka wrote:
>> > @@ -549,6 +549,11 @@ void *kmem_cache_alloc_lru_noprof(struct kmem_cache *s, struct list_lru *lru,
>> >  
>> >  void kmem_cache_free(struct kmem_cache *s, void *objp);
>> >  
>> > +kmem_buckets *kmem_buckets_create(const char *name, unsigned int align,
>> > +				  slab_flags_t flags,
>> > +				  unsigned int useroffset, unsigned int usersize,
>> > +				  void (*ctor)(void *));
>> 
>> I'd drop the ctor, I can't imagine how it would be used with variable-sized
>> allocations.
> 
> I've kept it because for "kmalloc wrapper" APIs, e.g. devm_kmalloc(),
> there is some "housekeeping" that gets done explicitly right now that I
> think would be better served by using a ctor in the future. These APIs
> are variable-sized, but have a fixed size header, so they have a
> "minimum size" that the ctor can still operate on, etc.
> 
>> Probably also "align" doesn't make much sense since we're just
>> copying the kmalloc cache sizes and its implicit alignment of any
>> power-of-two allocations.
> 
> Yeah, that's probably true. I kept it since I wanted to mirror
> kmem_cache_create() to make this API more familiar looking.

Rust people were asking about kmalloc alignment (but I forgot the details)
so maybe this could be useful for them? CC rust-for-linux.

>> I don't think any current kmalloc user would
>> suddenly need either of those as you convert it to buckets, and definitely
>> not any user converted automatically by the code tagging.
> 
> Right, it's not needed for either the explicit users nor the future
> automatic users. But since these arguments are available internally,
> there seems to be future utility,  it's not fast path, and it made things
> feel like the existing API, I'd kind of like to keep it.
> 
> But all that said, if you really don't want it, then sure I can drop
> those arguments. Adding them back in the future shouldn't be too
> much churn.

I guess we can keep it then.

Re: [PATCH v5 4/6] mm/slab: Introduce kmem_buckets_create() and family

[Boqun Feng]

On Thu, Jun 20, 2024 at 10:43:39PM +0200, Vlastimil Babka wrote:
> On 6/20/24 8:54 PM, Kees Cook wrote:
> > On Thu, Jun 20, 2024 at 03:56:27PM +0200, Vlastimil Babka wrote:
> >> > @@ -549,6 +549,11 @@ void *kmem_cache_alloc_lru_noprof(struct kmem_cache *s, struct list_lru *lru,
> >> >  
> >> >  void kmem_cache_free(struct kmem_cache *s, void *objp);
> >> >  
> >> > +kmem_buckets *kmem_buckets_create(const char *name, unsigned int align,
> >> > +				  slab_flags_t flags,
> >> > +				  unsigned int useroffset, unsigned int usersize,
> >> > +				  void (*ctor)(void *));
> >> 
> >> I'd drop the ctor, I can't imagine how it would be used with variable-sized
> >> allocations.
> > 
> > I've kept it because for "kmalloc wrapper" APIs, e.g. devm_kmalloc(),
> > there is some "housekeeping" that gets done explicitly right now that I
> > think would be better served by using a ctor in the future. These APIs
> > are variable-sized, but have a fixed size header, so they have a
> > "minimum size" that the ctor can still operate on, etc.
> > 
> >> Probably also "align" doesn't make much sense since we're just
> >> copying the kmalloc cache sizes and its implicit alignment of any
> >> power-of-two allocations.
> > 
> > Yeah, that's probably true. I kept it since I wanted to mirror
> > kmem_cache_create() to make this API more familiar looking.
> 
> Rust people were asking about kmalloc alignment (but I forgot the details)

It was me! The ask is whether we can specify the alignment for the
allocation API, for example, requesting a size=96 and align=32 memory,
or the allocation API could do a "best alignment", for example,
allocating a size=96 will give a align=32 memory. As far as I
understand, kmalloc() doesn't support this.

> so maybe this could be useful for them? CC rust-for-linux.
> 

I took a quick look as what kmem_buckets is, and seems to me that align
doesn't make sense here (and probably not useful in Rust as well)
because a kmem_buckets is a set of kmem_caches, each has its own object
size, making them share the same alignment is probably not what you
want. But I could be missing something.

Regards,
Boqun

> >> I don't think any current kmalloc user would
> >> suddenly need either of those as you convert it to buckets, and definitely
> >> not any user converted automatically by the code tagging.
> > 
> > Right, it's not needed for either the explicit users nor the future
> > automatic users. But since these arguments are available internally,
> > there seems to be future utility,  it's not fast path, and it made things
> > feel like the existing API, I'd kind of like to keep it.
> > 
> > But all that said, if you really don't want it, then sure I can drop
> > those arguments. Adding them back in the future shouldn't be too
> > much churn.
> 
> I guess we can keep it then.
> 

Re: [PATCH v5 4/6] mm/slab: Introduce kmem_buckets_create() and family

[Vlastimil Babka]

On 6/28/24 7:35 AM, Boqun Feng wrote:
> On Thu, Jun 20, 2024 at 10:43:39PM +0200, Vlastimil Babka wrote:
>> On 6/20/24 8:54 PM, Kees Cook wrote:
>> > On Thu, Jun 20, 2024 at 03:56:27PM +0200, Vlastimil Babka wrote:
>> >> > @@ -549,6 +549,11 @@ void *kmem_cache_alloc_lru_noprof(struct kmem_cache *s, struct list_lru *lru,
>> >> >  
>> >> >  void kmem_cache_free(struct kmem_cache *s, void *objp);
>> >> >  
>> >> > +kmem_buckets *kmem_buckets_create(const char *name, unsigned int align,
>> >> > +				  slab_flags_t flags,
>> >> > +				  unsigned int useroffset, unsigned int usersize,
>> >> > +				  void (*ctor)(void *));
>> >> 
>> >> I'd drop the ctor, I can't imagine how it would be used with variable-sized
>> >> allocations.
>> > 
>> > I've kept it because for "kmalloc wrapper" APIs, e.g. devm_kmalloc(),
>> > there is some "housekeeping" that gets done explicitly right now that I
>> > think would be better served by using a ctor in the future. These APIs
>> > are variable-sized, but have a fixed size header, so they have a
>> > "minimum size" that the ctor can still operate on, etc.
>> > 
>> >> Probably also "align" doesn't make much sense since we're just
>> >> copying the kmalloc cache sizes and its implicit alignment of any
>> >> power-of-two allocations.
>> > 
>> > Yeah, that's probably true. I kept it since I wanted to mirror
>> > kmem_cache_create() to make this API more familiar looking.
>> 
>> Rust people were asking about kmalloc alignment (but I forgot the details)
> 
> It was me! The ask is whether we can specify the alignment for the
> allocation API, for example, requesting a size=96 and align=32 memory,
> or the allocation API could do a "best alignment", for example,
> allocating a size=96 will give a align=32 memory. As far as I
> understand, kmalloc() doesn't support this.

Hm yeah we only have guarantees for power-or-2 allocations.

>> so maybe this could be useful for them? CC rust-for-linux.
>> 
> 
> I took a quick look as what kmem_buckets is, and seems to me that align
> doesn't make sense here (and probably not useful in Rust as well)
> because a kmem_buckets is a set of kmem_caches, each has its own object
> size, making them share the same alignment is probably not what you
> want. But I could be missing something.

How flexible do you need those alignments to be? Besides the power-of-two
guarantees, we currently have only two odd sizes with 96 and 192. If those
were guaranteed to be aligned 32 bytes, would that be sufficient? Also do
you ever allocate anything smaller than 32 bytes then?

To summarize, if Rust's requirements can be summarized by some rules and
it's not completely ad-hoc per-allocation alignment requirement (or if it
is, does it have an upper bound?) we could perhaps figure out the creation
of rust-specific kmem_buckets to give it what's needed?

> Regards,
> Boqun
> 
>> >> I don't think any current kmalloc user would
>> >> suddenly need either of those as you convert it to buckets, and definitely
>> >> not any user converted automatically by the code tagging.
>> > 
>> > Right, it's not needed for either the explicit users nor the future
>> > automatic users. But since these arguments are available internally,
>> > there seems to be future utility,  it's not fast path, and it made things
>> > feel like the existing API, I'd kind of like to keep it.
>> > 
>> > But all that said, if you really don't want it, then sure I can drop
>> > those arguments. Adding them back in the future shouldn't be too
>> > much churn.
>> 
>> I guess we can keep it then.
>> 

Re: [PATCH v5 4/6] mm/slab: Introduce kmem_buckets_create() and family

[Alice Ryhl]

On Fri, Jun 28, 2024 at 10:40 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 6/28/24 7:35 AM, Boqun Feng wrote:
> > On Thu, Jun 20, 2024 at 10:43:39PM +0200, Vlastimil Babka wrote:
> >> On 6/20/24 8:54 PM, Kees Cook wrote:
> >> > On Thu, Jun 20, 2024 at 03:56:27PM +0200, Vlastimil Babka wrote:
> >> >> > @@ -549,6 +549,11 @@ void *kmem_cache_alloc_lru_noprof(struct kmem_cache *s, struct list_lru *lru,
> >> >> >
> >> >> >  void kmem_cache_free(struct kmem_cache *s, void *objp);
> >> >> >
> >> >> > +kmem_buckets *kmem_buckets_create(const char *name, unsigned int align,
> >> >> > +                                 slab_flags_t flags,
> >> >> > +                                 unsigned int useroffset, unsigned int usersize,
> >> >> > +                                 void (*ctor)(void *));
> >> >>
> >> >> I'd drop the ctor, I can't imagine how it would be used with variable-sized
> >> >> allocations.
> >> >
> >> > I've kept it because for "kmalloc wrapper" APIs, e.g. devm_kmalloc(),
> >> > there is some "housekeeping" that gets done explicitly right now that I
> >> > think would be better served by using a ctor in the future. These APIs
> >> > are variable-sized, but have a fixed size header, so they have a
> >> > "minimum size" that the ctor can still operate on, etc.
> >> >
> >> >> Probably also "align" doesn't make much sense since we're just
> >> >> copying the kmalloc cache sizes and its implicit alignment of any
> >> >> power-of-two allocations.
> >> >
> >> > Yeah, that's probably true. I kept it since I wanted to mirror
> >> > kmem_cache_create() to make this API more familiar looking.
> >>
> >> Rust people were asking about kmalloc alignment (but I forgot the details)
> >
> > It was me! The ask is whether we can specify the alignment for the
> > allocation API, for example, requesting a size=96 and align=32 memory,
> > or the allocation API could do a "best alignment", for example,
> > allocating a size=96 will give a align=32 memory. As far as I
> > understand, kmalloc() doesn't support this.
>
> Hm yeah we only have guarantees for power-or-2 allocations.
>
> >> so maybe this could be useful for them? CC rust-for-linux.
> >>
> >
> > I took a quick look as what kmem_buckets is, and seems to me that align
> > doesn't make sense here (and probably not useful in Rust as well)
> > because a kmem_buckets is a set of kmem_caches, each has its own object
> > size, making them share the same alignment is probably not what you
> > want. But I could be missing something.
>
> How flexible do you need those alignments to be? Besides the power-of-two
> guarantees, we currently have only two odd sizes with 96 and 192. If those
> were guaranteed to be aligned 32 bytes, would that be sufficient? Also do
> you ever allocate anything smaller than 32 bytes then?
>
> To summarize, if Rust's requirements can be summarized by some rules and
> it's not completely ad-hoc per-allocation alignment requirement (or if it
> is, does it have an upper bound?) we could perhaps figure out the creation
> of rust-specific kmem_buckets to give it what's needed?

Rust's allocator API can take any size and alignment as long as:

1. The alignment is a power of two.
2. The size is non-zero.
3. When you round up the size to the next multiple of the alignment,
then it must not overflow the signed type isize / ssize_t.

What happens right now is that when Rust wants an allocation with a
higher alignment than ARCH_SLAB_MINALIGN, then it will increase size
until it becomes a power of two so that the power-of-two guarantee
gives a properly aligned allocation.

Alice

Re: [PATCH v5 4/6] mm/slab: Introduce kmem_buckets_create() and family

[Vlastimil Babka]

On 6/28/24 11:06 AM, Alice Ryhl wrote:
>> >>
>> >
>> > I took a quick look as what kmem_buckets is, and seems to me that align
>> > doesn't make sense here (and probably not useful in Rust as well)
>> > because a kmem_buckets is a set of kmem_caches, each has its own object
>> > size, making them share the same alignment is probably not what you
>> > want. But I could be missing something.
>>
>> How flexible do you need those alignments to be? Besides the power-of-two
>> guarantees, we currently have only two odd sizes with 96 and 192. If those
>> were guaranteed to be aligned 32 bytes, would that be sufficient? Also do
>> you ever allocate anything smaller than 32 bytes then?
>>
>> To summarize, if Rust's requirements can be summarized by some rules and
>> it's not completely ad-hoc per-allocation alignment requirement (or if it
>> is, does it have an upper bound?) we could perhaps figure out the creation
>> of rust-specific kmem_buckets to give it what's needed?
> 
> Rust's allocator API can take any size and alignment as long as:
> 
> 1. The alignment is a power of two.
> 2. The size is non-zero.
> 3. When you round up the size to the next multiple of the alignment,
> then it must not overflow the signed type isize / ssize_t.
> 
> What happens right now is that when Rust wants an allocation with a
> higher alignment than ARCH_SLAB_MINALIGN, then it will increase size
> until it becomes a power of two so that the power-of-two guarantee
> gives a properly aligned allocation.

So am I correct thinking that, if the cache of size 96 bytes guaranteed a
32byte alignment, and 192 bytes guaranteed 64byte alignment, and the rest of
sizes with the already guaranteed power-of-two alignment, then on rust side
you would only have to round up sizes to the next multiples of the alignemnt
(rule 3 above) and that would be sufficient?
 Abstracting from the specific sizes of 96 and 192, the guarantee on kmalloc
side would have to be - guarantee alignment to the largest power-of-two
divisor of the size. Does that sound right?

Then I think we could have some flag for kmem_buckets creation that would do
the right thing.

> Alice

Re: [PATCH v5 4/6] mm/slab: Introduce kmem_buckets_create() and family

[Alice Ryhl]

On Fri, Jun 28, 2024 at 11:17 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 6/28/24 11:06 AM, Alice Ryhl wrote:
> >> >>
> >> >
> >> > I took a quick look as what kmem_buckets is, and seems to me that align
> >> > doesn't make sense here (and probably not useful in Rust as well)
> >> > because a kmem_buckets is a set of kmem_caches, each has its own object
> >> > size, making them share the same alignment is probably not what you
> >> > want. But I could be missing something.
> >>
> >> How flexible do you need those alignments to be? Besides the power-of-two
> >> guarantees, we currently have only two odd sizes with 96 and 192. If those
> >> were guaranteed to be aligned 32 bytes, would that be sufficient? Also do
> >> you ever allocate anything smaller than 32 bytes then?
> >>
> >> To summarize, if Rust's requirements can be summarized by some rules and
> >> it's not completely ad-hoc per-allocation alignment requirement (or if it
> >> is, does it have an upper bound?) we could perhaps figure out the creation
> >> of rust-specific kmem_buckets to give it what's needed?
> >
> > Rust's allocator API can take any size and alignment as long as:
> >
> > 1. The alignment is a power of two.
> > 2. The size is non-zero.
> > 3. When you round up the size to the next multiple of the alignment,
> > then it must not overflow the signed type isize / ssize_t.
> >
> > What happens right now is that when Rust wants an allocation with a
> > higher alignment than ARCH_SLAB_MINALIGN, then it will increase size
> > until it becomes a power of two so that the power-of-two guarantee
> > gives a properly aligned allocation.
>
> So am I correct thinking that, if the cache of size 96 bytes guaranteed a
> 32byte alignment, and 192 bytes guaranteed 64byte alignment, and the rest of
> sizes with the already guaranteed power-of-two alignment, then on rust side
> you would only have to round up sizes to the next multiples of the alignemnt
> (rule 3 above) and that would be sufficient?
>  Abstracting from the specific sizes of 96 and 192, the guarantee on kmalloc
> side would have to be - guarantee alignment to the largest power-of-two
> divisor of the size. Does that sound right?
>
> Then I think we could have some flag for kmem_buckets creation that would do
> the right thing.

If kmalloc/krealloc guarantee that an allocation is aligned according
to the largest power-of-two divisor of the size, then the Rust
allocator would definitely be simplified as we would not longer need
this part:

if layout.align() > bindings::ARCH_SLAB_MINALIGN {
    // The alignment requirement exceeds the slab guarantee, thus try
to enlarge the size
    // to use the "power-of-two" size/alignment guarantee (see
comments in `kmalloc()` for
    // more information).
    //
    // Note that `layout.size()` (after padding) is guaranteed to be a
multiple of
    // `layout.align()`, so `next_power_of_two` gives enough alignment
guarantee.
    size = size.next_power_of_two();
}

We would only need to keep the part that rounds up the size to a
multiple of the alignment.

Alice

Re: [PATCH v5 4/6] mm/slab: Introduce kmem_buckets_create() and family

[Kees Cook]

On Thu, Jun 27, 2024 at 10:35:36PM -0700, Boqun Feng wrote:
> On Thu, Jun 20, 2024 at 10:43:39PM +0200, Vlastimil Babka wrote:
> > Rust people were asking about kmalloc alignment (but I forgot the details)
> 
> It was me! The ask is whether we can specify the alignment for the
> allocation API, for example, requesting a size=96 and align=32 memory,
> or the allocation API could do a "best alignment", for example,
> allocating a size=96 will give a align=32 memory. As far as I
> understand, kmalloc() doesn't support this.

I can drop the "align" argument. Do we want to hard-code a
per-cache-size alignment for the caches in a kmem_buckets collection?

-- 
Kees Cook

Re: [PATCH v5 4/6] mm/slab: Introduce kmem_buckets_create() and family

[Vlastimil Babka]

On 6/28/24 5:47 PM, Kees Cook wrote:
> On Thu, Jun 27, 2024 at 10:35:36PM -0700, Boqun Feng wrote:
>> On Thu, Jun 20, 2024 at 10:43:39PM +0200, Vlastimil Babka wrote:
>> > Rust people were asking about kmalloc alignment (but I forgot the details)
>> 
>> It was me! The ask is whether we can specify the alignment for the
>> allocation API, for example, requesting a size=96 and align=32 memory,
>> or the allocation API could do a "best alignment", for example,
>> allocating a size=96 will give a align=32 memory. As far as I
>> understand, kmalloc() doesn't support this.
> 
> I can drop the "align" argument. Do we want to hard-code a
> per-cache-size alignment for the caches in a kmem_buckets collection?

I think you can drop it as a single value is really ill suited for a
collection of different sizes.

As for Rust's requirements we could consider whether to add a special flag
if they use own bucket, or just implement the rules for non-power-of-two
size globally. It should be feasible as I think in the non-debug caches it
shouldn't in fact change the existing layout, which is the same situation as
when we codified the power-of-two caches alignment as guaranteed.