Messages in this thread |  | | From | Rob Clark <> | | Date | Wed, 3 Jun 2026 04:13:49 -0700 | | Subject | Re: [PATCH] mm/slab: improve kmem_cache_alloc_bulk |
| |
On Wed, Jun 3, 2026 at 2:17 AM Vlastimil Babka (SUSE) <vbabka@kernel.org> wrote: > > On 6/1/26 16:39, Rob Clark wrote: > > On Mon, Jun 1, 2026 at 6:32 AM Rob Clark <rob.clark@oss.qualcomm.com> wrote: > >> > >> On Mon, Jun 1, 2026 at 5:50 AM Vlastimil Babka (SUSE) <vbabka@kernel.org> 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.
Thanks
> 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.
Correct, the job is not reused. But I suppose setting p->pages to NULL would make things more obvious, so no objection to that.
BR, -R
> > + > > 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; >
|  |