Re: [PATCH] mm/slab: improve kmem_cache_alloc_bulk

On 6/1/26 16:39, Rob Clark wrote:
> On Mon, Jun 1, 2026 at 6:32 AM Rob Clark <[email protected]> wrote:
>>
>> On Mon, Jun 1, 2026 at 5:50 AM Vlastimil Babka (SUSE) <[email protected]> wrote:
>> >
>> > On 6/1/26 13:38, Christoph Hellwig wrote:
>> > > On Mon, Jun 01, 2026 at 10:16:30AM +0200, Vlastimil Babka (SUSE) wrote:
>> > >> > kmem_cache_alloc_bulk() returning 0 was considered a success in that case.
>> > >> >
>> > >> > Either fixing kmem_cache_alloc_bulk() (and the comment) or fixing the
>> > >> > user sounds fine to me.
>> > >>
>> > >> Would it be wrong if we just returned true for size of 0? Would something
>> > >> else break?
>> > >
>> > > I don't think it is wrong per se, but it feels like the wrong kind of
>> > > API.  I.e. I don't think the MSM caller actually wants this, as they'd
>> > > also do a zero-sized kvmalloc.
>> >
>> > If p->count is 0 then indeed there's a zero-sized kvmalloc so p->pages ==
>> > ZERO_SIZE_PTR but then nothing breaks because nothing tries to dereference it?
>> >
>> > msm_iommu_pagetable_prealloc_cleanup() has a "if (p->count > 0)" branch so
>> > it seems it's considered possible. But then the rest of the functions also
>> > seems working fine, i.e. kmem_cache_free_bulk() of zero size does nothing,
>> > kvfree() of ZERO_SIZE_PTR does nothing.
>> >
>> > It seems to me kmem_cache_alloc_bulk() returning true for size == 0 fits
>> > naturally in this world and is less likely to result in a gotcha?
>>
>> I think I was probably expecting kvmalloc(0) => NULL ... but it
>> happened to work out before
>>
>> Adding an "if (!p->count) return 0;" at the top of
>> msm_iommu_pagetable_prealloc_allocate() seems like the thing to do..
>> if you want, I can send that patch (but traveling this week... so
>> let's see what I can do)
> 
> Aaaaaand.. sending patch from hotel wifi doesn't seem to be a thing
> that works.. but I've tested the following w/ deqp-vk cts, and works
> as expected

Thanks, I will amend the commit in slab tree with this as the easiest way to
go foward.
Just a quick question though:

> ----------------
> 
> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index 7d449e5202c5..ef744d154bbe 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -332,13 +332,16 @@ msm_iommu_pagetable_prealloc_allocate(struct
> msm_mmu *mmu, struct msm_mmu_preall
>         struct kmem_cache *pt_cache = get_pt_cache(mmu);
>         int ret;
> 
> +       if (!p->count)
> +               return 0;

We know p->pages is NULL in this case, right? Because it was allocated by
vm_bind_job_create() using kzalloc().
And the job can't be reused with a leftover value?
(msm_iommu_pagetable_prealloc_cleanup doesn't set p->pages to zero).
Or should we set p->pages to NULL here.

> +
>         p->pages = kvmalloc_objs(*p->pages, p->count);
>         if (!p->pages)
>                 return -ENOMEM;
> 
>         ret = kmem_cache_alloc_bulk(pt_cache, GFP_KERNEL, p->count, p->pages);
>         if (ret != p->count) {
> -               kfree(p->pages);
> +               kvfree(p->pages);
>                 p->pages = NULL;
>                 p->count = ret;
>                 return -ENOMEM;