| Message ID | 984104c118a451fc4afa2eadb7206065f13b7af2.1638308023.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan, vmalloc, arm64: add vmalloc tagging support for SW/HW_TAGS | expand |
On Tue, Nov 30, 2021 at 10:41 PM <andrey.konovalov@linux.dev> wrote: > > From: Andrey Konovalov <andreyknvl@google.com> > > This patch separates code for zeroing memory from the code clearing tags > in post_alloc_hook(). > > This patch is not useful by itself but makes the simplifications in > the following patches easier to follow. > > This patch does no functional changes. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > --- > mm/page_alloc.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 2ada09a58e4b..0561cdafce36 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2406,19 +2406,21 @@ inline void post_alloc_hook(struct page *page, unsigned int order, > kasan_alloc_pages(page, order, gfp_flags); > } else { > bool init = !want_init_on_free() && want_init_on_alloc(gfp_flags); > + bool init_tags = init && (gfp_flags & __GFP_ZEROTAGS); > > kasan_unpoison_pages(page, order, init); > > - if (init) { > - if (gfp_flags & __GFP_ZEROTAGS) { > - int i; > + if (init_tags) { > + int i; > > - for (i = 0; i < 1 << order; i++) > - tag_clear_highpage(page + i); > - } else { > - kernel_init_free_pages(page, 1 << order); > - } > + for (i = 0; i < 1 << order; i++) > + tag_clear_highpage(page + i); > + > + init = false; I find this a bit twisted and prone to breakages. Maybe just check for (init && !init_tags) below? > } > + > + if (init) > + kernel_init_free_pages(page, 1 << order); > } > > set_page_owner(page, order, gfp_flags); > -- > 2.25.1 >
On Thu, Dec 2, 2021 at 5:14 PM Alexander Potapenko <glider@google.com> wrote: > > On Tue, Nov 30, 2021 at 10:41 PM <andrey.konovalov@linux.dev> wrote: > > > > From: Andrey Konovalov <andreyknvl@google.com> > > > > This patch separates code for zeroing memory from the code clearing tags > > in post_alloc_hook(). > > > > This patch is not useful by itself but makes the simplifications in > > the following patches easier to follow. > > > > This patch does no functional changes. > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > --- > > mm/page_alloc.c | 18 ++++++++++-------- > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 2ada09a58e4b..0561cdafce36 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -2406,19 +2406,21 @@ inline void post_alloc_hook(struct page *page, unsigned int order, > > kasan_alloc_pages(page, order, gfp_flags); > > } else { > > bool init = !want_init_on_free() && want_init_on_alloc(gfp_flags); > > + bool init_tags = init && (gfp_flags & __GFP_ZEROTAGS); > > > > kasan_unpoison_pages(page, order, init); > > > > - if (init) { > > - if (gfp_flags & __GFP_ZEROTAGS) { > > - int i; > > + if (init_tags) { > > + int i; > > > > - for (i = 0; i < 1 << order; i++) > > - tag_clear_highpage(page + i); > > - } else { > > - kernel_init_free_pages(page, 1 << order); > > - } > > + for (i = 0; i < 1 << order; i++) > > + tag_clear_highpage(page + i); > > + > > + init = false; > > I find this a bit twisted and prone to breakages. > Maybe just check for (init && !init_tags) below? I did it this way deliberately. Check out the code after all the changes: https://github.com/xairy/linux/blob/up-kasan-vmalloc-tags-v1/mm/page_alloc.c#L2447 It's possible to remove resetting the init variable by expanding the if (init) check listing all conditions under which init is currently reset, but that would essentially be duplicating the checks. I think resetting init is more clear. Please let me know what you think. Thanks!
On Mon, Dec 6, 2021 at 10:09 PM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > On Thu, Dec 2, 2021 at 5:14 PM Alexander Potapenko <glider@google.com> wrote: > > > > On Tue, Nov 30, 2021 at 10:41 PM <andrey.konovalov@linux.dev> wrote: > > > > > > From: Andrey Konovalov <andreyknvl@google.com> > > > > > > This patch separates code for zeroing memory from the code clearing tags > > > in post_alloc_hook(). > > > > > > This patch is not useful by itself but makes the simplifications in > > > the following patches easier to follow. > > > > > > This patch does no functional changes. > > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > --- > > > mm/page_alloc.c | 18 ++++++++++-------- > > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > > index 2ada09a58e4b..0561cdafce36 100644 > > > --- a/mm/page_alloc.c > > > +++ b/mm/page_alloc.c > > > @@ -2406,19 +2406,21 @@ inline void post_alloc_hook(struct page *page, unsigned int order, > > > kasan_alloc_pages(page, order, gfp_flags); > > > } else { > > > bool init = !want_init_on_free() && want_init_on_alloc(gfp_flags); > > > + bool init_tags = init && (gfp_flags & __GFP_ZEROTAGS); > > > > > > kasan_unpoison_pages(page, order, init); > > > > > > - if (init) { > > > - if (gfp_flags & __GFP_ZEROTAGS) { > > > - int i; > > > + if (init_tags) { > > > + int i; > > > > > > - for (i = 0; i < 1 << order; i++) > > > - tag_clear_highpage(page + i); > > > - } else { > > > - kernel_init_free_pages(page, 1 << order); > > > - } > > > + for (i = 0; i < 1 << order; i++) > > > + tag_clear_highpage(page + i); > > > + > > > + init = false; > > > > I find this a bit twisted and prone to breakages. > > Maybe just check for (init && !init_tags) below? > > I did it this way deliberately. Check out the code after all the changes: > > https://github.com/xairy/linux/blob/up-kasan-vmalloc-tags-v1/mm/page_alloc.c#L2447 > > It's possible to remove resetting the init variable by expanding the > if (init) check listing all conditions under which init is currently > reset, but that would essentially be duplicating the checks. I think > resetting init is more clear. > > Please let me know what you think. Ah, I see, so there are more cases in which you set init = false. Fine then.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 2ada09a58e4b..0561cdafce36 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2406,19 +2406,21 @@ inline void post_alloc_hook(struct page *page, unsigned int order, kasan_alloc_pages(page, order, gfp_flags); } else { bool init = !want_init_on_free() && want_init_on_alloc(gfp_flags); + bool init_tags = init && (gfp_flags & __GFP_ZEROTAGS); kasan_unpoison_pages(page, order, init); - if (init) { - if (gfp_flags & __GFP_ZEROTAGS) { - int i; + if (init_tags) { + int i; - for (i = 0; i < 1 << order; i++) - tag_clear_highpage(page + i); - } else { - kernel_init_free_pages(page, 1 << order); - } + for (i = 0; i < 1 << order; i++) + tag_clear_highpage(page + i); + + init = false; } + + if (init) + kernel_init_free_pages(page, 1 << order); } set_page_owner(page, order, gfp_flags);