On Tue, Dec 21, 2021 at 12:58:14AM +0100, Vlastimil Babka wrote: > On 12/16/21 16:00, Hyeonggon Yoo wrote: > > On Tue, Dec 14, 2021 at 01:57:22PM +0100, Vlastimil Babka wrote: > >> On 12/1/21 19:14, Vlastimil Babka wrote: > >> > Folks from non-slab subsystems are Cc'd only to patches affecting them, and > >> > this cover letter. > >> > > >> > Series also available in git, based on 5.16-rc3: > >> > https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/linux.git/log/?h=slab-struct_slab-v2r2 > >> > >> Pushed a new branch slab-struct-slab-v3r3 with accumulated fixes and small tweaks > >> and a new patch from Hyeonggon Yoo on top. To avoid too much spam, here's a range diff: > > > > Reviewing the whole patch series takes longer than I thought. > > I'll try to review and test rest of patches when I have time. > > > > I added Tested-by if kernel builds okay and kselftests > > does not break the kernel on my machine. > > (with CONFIG_SLAB/SLUB/SLOB depending on the patch), > > Thanks! > :) > > Let me know me if you know better way to test a patch. > > Testing on your machine is just fine. > Good! > > # mm/slub: Define struct slab fields for CONFIG_SLUB_CPU_PARTIAL only when enabled > > > > Reviewed-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > Tested-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > > > Comment: > > Works on both SLUB_CPU_PARTIAL and !SLUB_CPU_PARTIAL. > > btw, do we need slabs_cpu_partial attribute when we don't use > > cpu partials? (!SLUB_CPU_PARTIAL) > > The sysfs attribute? Yeah we should be consistent to userspace expecting to > read it (even with zeroes), regardless of config. > I thought entirely disabling the attribute is simpler, But okay If it should be exposed even if it's always zero. > > # mm/slub: Simplify struct slab slabs field definition > > Comment: > > > > This is how struct page looks on the top of v3r3 branch: > > struct page { > > [...] > > struct { /* slab, slob and slub */ > > union { > > struct list_head slab_list; > > struct { /* Partial pages */ > > struct page *next; > > #ifdef CONFIG_64BIT > > int pages; /* Nr of pages left */ > > #else > > short int pages; > > #endif > > }; > > }; > > [...] > > > > It's not consistent with struct slab. > > Hm right. But as we don't actually use the struct page version anymore, and > it's not one of the fields checked by SLAB_MATCH(), we can ignore this. > Yeah this is not a big problem. just mentioned this because it looked weird and I didn't know when the patch "mm: Remove slab from struct page" will come back. > > I think this is because "mm: Remove slab from struct page" was dropped. > > That was just postponed until iommu changes are in. Matthew mentioned those > might be merged too, so that final cleanup will happen too and take care of > the discrepancy above, so no need for extra churn to address it speficially. > Okay it seems no extra work needed until the iommu changes are in! BTW, in the patch (that I sent) ("mm/slob: Remove unnecessary page_mapcount_reset() function call"), it refers commit 4525180926f9 ("mm/sl*b: Differentiate struct slab fields by sl*b implementations"). But the commit hash 4525180926f9 changed after the tree has been changed. It will be nice to write a script to handle situations like this. > > Would you update some of patches? > > > > # mm/sl*b: Differentiate struct slab fields by sl*b implementations > > Reviewed-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > Tested-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > Works SL[AUO]B on my machine and makes code much better. > > > > # mm/slob: Convert SLOB to use struct slab and struct folio > > Reviewed-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > Tested-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > It still works fine on SLOB. > > > > # mm/slab: Convert kmem_getpages() and kmem_freepages() to struct slab > > Reviewed-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > Tested-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > > > # mm/slub: Convert __free_slab() to use struct slab > > Reviewed-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > Tested-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > > > > Thanks, > > Hyeonggon. > > Thanks again, > Vlastimil Have a nice day, thanks! Hyeonggon.
