[RFC][DISCUSSION] dma-mapping: allocating noncoherent buffer without mapping
Last month we discussed drivers that uses kmalloc(GFP_DMA) for noncoherent mapping should be converted to use DMA API [1]. Simple grep with GFP_DMA shows that many of drivers are mistakenly using GFP_DMA flag. So our purpose was to make DMA API choose right zone depending on device's dma mask. Baoquan and I are trying to make drivers to use dma_alloc_noncoherent() when allocating the buffer. But it's not simple for some of drivers; there is a gap between dma_alloc_noncoherent() and the previous convention (allocating buffer from buddy or slab allocator and mapping it when needed.) For example, some drivers allocate buffer and reuse it. it just maps and unmaps whenever needed. And some drivers does not even maps the buffer. (some drivers that does not use DMA API) So I think we need to implement a version of dma_alloc_noncoherent() that does not map the buffer. I think implementing a helper that internally calls __dma_direct_alloc_pages() will be okay. As I'm not expert in this area, I want to hear others' opinions. [1] https://lkml.org/lkml/2021/12/13/1121 Thanks, Hyeonggon. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 00/33] Separate struct slab from struct page
On Wed, Dec 22, 2021 at 05:56:50PM +0100, Vlastimil Babka wrote:
> On 12/14/21 13:57, 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:
>
> Hi, I've pushed another update branch slab-struct_slab-v4r1, and also to
> -next. I've shortened git commit log lines to make checkpatch happier,
> so no range-diff as it would be too long. I believe it would be useless
> spam to post the whole series now, shortly before xmas, so I will do it
> at rc8 time, to hopefully collect remaining reviews. But if anyone wants
> a mailed version, I can do that.
>
Hello Matthew and Vlastimil.
it's part 3 of review.
# mm: Convert struct page to struct slab in functions used by other subsystems
Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
# mm/slub: Convert most struct page to struct slab by spatch
Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
with a question below.
-static int check_slab(struct kmem_cache *s, struct page *page)
+static int check_slab(struct kmem_cache *s, struct slab *slab)
{
int maxobj;
- if (!PageSlab(page)) {
- slab_err(s, page, "Not a valid slab page");
+ if (!folio_test_slab(slab_folio(slab))) {
+ slab_err(s, slab, "Not a valid slab page");
return 0;
}
Can't we guarantee that struct slab * always points to a slab?
for struct page * it can be !PageSlab(page) because struct page *
can be other than slab. but struct slab * can only be slab
unlike struct page. code will be simpler if we guarantee that
struct slab * always points to a slab (or NULL).
# mm/slub: Convert pfmemalloc_match() to take a struct slab
It's confusing to me because the original pfmemalloc_match() is removed
and pfmemalloc_match_unsafe() was renamed to pfmemalloc_match() and
converted to use slab_test_pfmemalloc() helper.
But I agree with the resulting code. so:
Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
# mm/slub: Convert alloc_slab_page() to return a struct slab
Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
# mm/slub: Convert print_page_info() to print_slab_info()
Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
I hope to review rest of patches in a week.
Thanks,
Hyeonggon
> Changes in v4:
> - rebase to 5.16-rc6 to avoid a conflict with mainline
> - collect acks/reviews/tested-by from Johannes, Roman, Hyeonggon Yoo -
> thanks!
> - in patch "mm/slub: Convert detached_freelist to use a struct slab"
> renamed free_nonslab_page() to free_large_kmalloc() and use folio there,
> as suggested by Roman
> - in "mm/memcg: Convert slab objcgs from struct page to struct slab"
> change one caller of slab_objcgs_check() to slab_objcgs() as suggested
> by Johannes, realize the other caller should be also changed, and remove
> slab_objcgs_check() completely.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 00/33] Separate struct slab from struct page
On Sat, Dec 25, 2021 at 05:53:23PM +, Matthew Wilcox wrote:
> On Sat, Dec 25, 2021 at 09:16:55AM +0000, Hyeonggon Yoo wrote:
> > # mm: Convert struct page to struct slab in functions used by other
> > subsystems
> > I'm not familiar with kasan, but to ask:
> > Does kasan_slab_free detect invalid free if someone frees
> > an object that is not allocated from slab?
> >
> > @@ -341,7 +341,7 @@ static inline bool kasan_slab_free(struct
> > kmem_cache *cache, void *object,
> > - if (unlikely(nearest_obj(cache, virt_to_head_page(object), object)
> > !=
> > + if (unlikely(nearest_obj(cache, virt_to_slab(object), object) !=
> > object)) {
> > kasan_report_invalid_free(tagged_object, ip);
> > return true;
> >
> > I'm asking this because virt_to_slab() will return NULL if folio_test_slab()
> > returns false. That will cause NULL pointer dereference in nearest_obj.
> > I don't think this change is intended.
>
> You need to track down how this could happen. As far as I can tell,
> it's always called when we know the object is part of a slab. That's
> where the cachep pointer is deduced from.
Thank you Matthew, you are right. I read the code too narrowly.
when we call kasan hooks, we know that the object is allocated from
the slab cache. (through cache_from_obj)
I'll review that patch again in part 3!
Thanks,
Hyeonggon
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 00/33] Separate struct slab from struct page
On Wed, Dec 22, 2021 at 05:56:50PM +0100, Vlastimil Babka wrote:
> On 12/14/21 13:57, 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:
>
> Hi, I've pushed another update branch slab-struct_slab-v4r1, and also to
> -next. I've shortened git commit log lines to make checkpatch happier,
> so no range-diff as it would be too long. I believe it would be useless
> spam to post the whole series now, shortly before xmas, so I will do it
> at rc8 time, to hopefully collect remaining reviews. But if anyone wants
> a mailed version, I can do that.
Hello Vlastimil, Merry Christmas!
This is part 2 of reviewing/testing patches.
# mm/kasan: Convert to struct folio and struct slab
I'm not familiar with kasan yet but kasan runs well on my machine and
kasan's bug report functionality too works fine.
Tested-by: Hyeongogn Yoo <42.hye...@gmail.com>
# mm: Convert struct page to struct slab in functions used by other subsystems
I'm not familiar with kasan, but to ask:
Does kasan_slab_free detect invalid free if someone frees
an object that is not allocated from slab?
@@ -341,7 +341,7 @@ static inline bool kasan_slab_free(struct kmem_cache
*cache, void *object,
- if (unlikely(nearest_obj(cache, virt_to_head_page(object), object) !=
+ if (unlikely(nearest_obj(cache, virt_to_slab(object), object) !=
object)) {
kasan_report_invalid_free(tagged_object, ip);
return true;
I'm asking this because virt_to_slab() will return NULL if folio_test_slab()
returns false. That will cause NULL pointer dereference in nearest_obj.
I don't think this change is intended.
This makes me think some of virt_to_head_page() -> virt_to_slab()
conversion need to be reviewed with caution.
# mm/slab: Finish struct page to struct slab conversion
Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
# mm/slab: Convert most struct page to struct slab by spatch
Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
I'll come back with part 3 :)
Enjoy your Christmas!
Hyeonggon
> Changes in v4:
> - rebase to 5.16-rc6 to avoid a conflict with mainline
> - collect acks/reviews/tested-by from Johannes, Roman, Hyeonggon Yoo -
> thanks!
> - in patch "mm/slub: Convert detached_freelist to use a struct slab"
> renamed free_nonslab_page() to free_large_kmalloc() and use folio there,
> as suggested by Roman
> - in "mm/memcg: Convert slab objcgs from struct page to struct slab"
> change one caller of slab_objcgs_check() to slab_objcgs() as suggested
> by Johannes, realize the other caller should be also changed, and remove
> slab_objcgs_check() completely.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 00/33] Separate struct slab from struct page
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.hye...@gmail.com>
> > Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
> >
> > 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.hye...@gmail.com>
> > Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
> > 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.hye...@gmail.com>
> > Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
> > It still works fine on SLOB.
> >
> > # mm/slab: Convert kmem_getpages() and kmem_freepages() to struct slab
> > Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
> > Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
> >
> > # mm/slub: Convert __free_slab() to use struct slab
> > Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
> > Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
> >
> > Thanks,
> > Hyeonggon.
>
> Thanks again,
> Vlastimil
Have a nice day, thanks!
Hyeonggon.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 00/33] Separate struct slab from struct page
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),
Let me know me if you know better way to test a patch.
# mm/slub: Define struct slab fields for CONFIG_SLUB_CPU_PARTIAL only when
enabled
Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
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)
# 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.
I think this is because "mm: Remove slab from struct page" was dropped.
Would you update some of patches?
# mm/sl*b: Differentiate struct slab fields by sl*b implementations
Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
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.hye...@gmail.com>
Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
It still works fine on SLOB.
# mm/slab: Convert kmem_getpages() and kmem_freepages() to struct slab
Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
# mm/slub: Convert __free_slab() to use struct slab
Reviewed-by: Hyeonggon Yoo <42.hye...@gmail.com>
Tested-by: Hyeonggon Yoo <42.hye...@gmail.com>
Thanks,
Hyeonggon.
>
> 1: 10b656f9eb1e = 1: 10b656f9eb1e mm: add virt_to_folio() and
> folio_address()
> 2: 5e6ad846acf1 = 2: 5e6ad846acf1 mm/slab: Dissolve slab_map_pages() in
> its caller
> 3: 48d4e9407aa0 = 3: 48d4e9407aa0 mm/slub: Make object_err() static
> 4: fe1e19081321 = 4: fe1e19081321 mm: Split slab into its own type
> 5: af7fd46fbb9b = 5: af7fd46fbb9b mm: Add account_slab() and
> unaccount_slab()
> 6: 7ed088d601d9 = 6: 7ed088d601d9 mm: Convert virt_to_cache() to use
> struct slab
> 7: 1d41188b9401 = 7: 1d41188b9401 mm: Convert __ksize() to struct slab
> 8: 5d9d1231461f ! 8: 8fd22e0b086e mm: Use struct slab in kmem_obj_info()
> @@ Commit message
> slab type instead of the page type, we make it obvious that this can
> only be called for slabs.
>
> +[ vba...@suse.cz: also convert the related kmem_valid_obj() to
> folios ]
> +
> Signed-off-by: Matthew Wilcox (Oracle)
> Signed-off-by: Vlastimil Babka
>
> @@ mm/slab.h: struct kmem_obj_info {
> #endif /* MM_SLAB_H */
>
> ## mm/slab_common.c ##
> +@@ mm/slab_common.c: bool slab_is_available(void)
> + */
> + bool kmem_valid_obj(void *object)
> + {
> +-struct page *page;
> ++struct folio *folio;
> +
> + /* Some arches consider ZERO_SIZE_PTR to be a valid address. */
> + if (object < (void *)PAGE_SIZE || !virt_addr_valid(object))
> + return false;
> +-page = virt_to_head_page(object);
> +-return PageSlab(page);
> ++folio = virt_to_folio(object);
> ++return folio_test_slab(folio);
> + }
> + EXPORT_SYMBOL_GPL(kmem_valid_obj);
> +
> @@ mm/slab_common.c: void kmem_dump_obj(void *object)
> {
> char *cp = IS_ENABLED(CONFIG_MMU) ? "" : "/vmalloc";
> @@ mm/slub.c: int __kmem_cache_shu
Re: [PATCH v2 00/33] Separate struct slab from struct page
On Tue, Dec 14, 2021 at 03:43:35PM +0100, Vlastimil Babka wrote: > On 12/14/21 15:38, 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: > >> > > > > Hello Vlastimil, Thank you for nice work. > > I'm going to review and test new version soon in free time. > > Thanks! > You're welcome! > > Btw, I gave you some review and test tags and seems to be missing in new > > series. Did I do review/test process wrongly? It's first time to review > > patches so please let me know if I did it wrongly. > > You did right, sorry! I didn't include them as those were for patches that I > was additionally changing after your review/test and the decision what is > substantial change enough to need a new test/review is often fuzzy. Ah, Okay. review/test becomes invalid after some changing. that's okay. I was just unfamiliar with the process. Thank you! > So if you can recheck the new versions it would be great and then I will pick > that > up, thanks! Okay. I'll new versions. > > > -- > > Thank you. > > Hyeonggon. > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 00/33] Separate struct slab from struct page
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: > Hello Vlastimil, Thank you for nice work. I'm going to review and test new version soon in free time. Btw, I gave you some review and test tags and seems to be missing in new series. Did I do review/test process wrongly? It's first time to review patches so please let me know if I did it wrongly. -- Thank you. Hyeonggon. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
