| Message ID | 20240221194052.927623-20-surenb@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Memory allocation profiling | expand |
On 2/21/24 20:40, Suren Baghdasaryan wrote: > When a high-order page is split into smaller ones, each newly split > page should get its codetag. The original codetag is reused for these > pages but it's recorded as 0-byte allocation because original codetag > already accounts for the original high-order allocated page. This was v3 but then you refactored (for the better) so the commit log could reflect it? > Signed-off-by: Suren Baghdasaryan <surenb@google.com> I was going to R-b, but now I recalled the trickiness of __free_pages() for non-compound pages if it loses the race to a speculative reference. Will the codetag handling work fine there? > --- > include/linux/pgalloc_tag.h | 30 ++++++++++++++++++++++++++++++ > mm/huge_memory.c | 2 ++ > mm/page_alloc.c | 2 ++ > 3 files changed, 34 insertions(+) > > diff --git a/include/linux/pgalloc_tag.h b/include/linux/pgalloc_tag.h > index b49ab955300f..9e6ad8e0e4aa 100644 > --- a/include/linux/pgalloc_tag.h > +++ b/include/linux/pgalloc_tag.h > @@ -67,11 +67,41 @@ static inline void pgalloc_tag_sub(struct page *page, unsigned int order) > } > } > > +static inline void pgalloc_tag_split(struct page *page, unsigned int nr) > +{ > + int i; > + struct page_ext *page_ext; > + union codetag_ref *ref; > + struct alloc_tag *tag; > + > + if (!mem_alloc_profiling_enabled()) > + return; > + > + page_ext = page_ext_get(page); > + if (unlikely(!page_ext)) > + return; > + > + ref = codetag_ref_from_page_ext(page_ext); > + if (!ref->ct) > + goto out; > + > + tag = ct_to_alloc_tag(ref->ct); > + page_ext = page_ext_next(page_ext); > + for (i = 1; i < nr; i++) { > + /* Set new reference to point to the original tag */ > + alloc_tag_ref_set(codetag_ref_from_page_ext(page_ext), tag); > + page_ext = page_ext_next(page_ext); > + } > +out: > + page_ext_put(page_ext); > +} > + > #else /* CONFIG_MEM_ALLOC_PROFILING */ > > static inline void pgalloc_tag_add(struct page *page, struct task_struct *task, > unsigned int order) {} > static inline void pgalloc_tag_sub(struct page *page, unsigned int order) {} > +static inline void pgalloc_tag_split(struct page *page, unsigned int nr) {} > > #endif /* CONFIG_MEM_ALLOC_PROFILING */ > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 94c958f7ebb5..86daae671319 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -38,6 +38,7 @@ > #include <linux/sched/sysctl.h> > #include <linux/memory-tiers.h> > #include <linux/compat.h> > +#include <linux/pgalloc_tag.h> > > #include <asm/tlb.h> > #include <asm/pgalloc.h> > @@ -2899,6 +2900,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, > /* Caller disabled irqs, so they are still disabled here */ > > split_page_owner(head, nr); > + pgalloc_tag_split(head, nr); > > /* See comment in __split_huge_page_tail() */ > if (PageAnon(head)) { > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 58c0e8b948a4..4bc5b4720fee 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2621,6 +2621,7 @@ void split_page(struct page *page, unsigned int order) > for (i = 1; i < (1 << order); i++) > set_page_refcounted(page + i); > split_page_owner(page, 1 << order); > + pgalloc_tag_split(page, 1 << order); > split_page_memcg(page, 1 << order); > } > EXPORT_SYMBOL_GPL(split_page); > @@ -4806,6 +4807,7 @@ static void *make_alloc_exact(unsigned long addr, unsigned int order, > struct page *last = page + nr; > > split_page_owner(page, 1 << order); > + pgalloc_tag_split(page, 1 << order); > split_page_memcg(page, 1 << order); > while (page < --last) > set_page_refcounted(last);
On Tue, Feb 27, 2024 at 2:10 AM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 2/21/24 20:40, Suren Baghdasaryan wrote: > > When a high-order page is split into smaller ones, each newly split > > page should get its codetag. The original codetag is reused for these > > pages but it's recorded as 0-byte allocation because original codetag > > already accounts for the original high-order allocated page. > > This was v3 but then you refactored (for the better) so the commit log > could reflect it? Yes, technically mechnism didn't change but I should word it better. Smth like this: When a high-order page is split into smaller ones, each newly split page should get its codetag. After the split each split page will be referencing the original codetag. The codetag's "bytes" counter remains the same because the amount of allocated memory has not changed, however the "calls" counter gets increased to keep the counter correct when these individual pages get freed. > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > I was going to R-b, but now I recalled the trickiness of > __free_pages() for non-compound pages if it loses the race to a > speculative reference. Will the codetag handling work fine there? I think so. Each non-compoud page has its individual reference to its codetag and will decrement it whenever the page is freed. IIUC the logic in __free_pages(), when it loses race to a speculative reference it will free all pages except for the first one and the first one will be freed when the last put_page() happens. If prior to this all these pages were split from one page then all of them will have their own reference which points to the same codetag. Every time one of these pages are freed that codetag's "bytes" and "calls" counters will be decremented. I think accounting will work correctly irrespective of where these pages are freed, in __free_pages() or by put_page(). > > > --- > > include/linux/pgalloc_tag.h | 30 ++++++++++++++++++++++++++++++ > > mm/huge_memory.c | 2 ++ > > mm/page_alloc.c | 2 ++ > > 3 files changed, 34 insertions(+) > > > > diff --git a/include/linux/pgalloc_tag.h b/include/linux/pgalloc_tag.h > > index b49ab955300f..9e6ad8e0e4aa 100644 > > --- a/include/linux/pgalloc_tag.h > > +++ b/include/linux/pgalloc_tag.h > > @@ -67,11 +67,41 @@ static inline void pgalloc_tag_sub(struct page *page, unsigned int order) > > } > > } > > > > +static inline void pgalloc_tag_split(struct page *page, unsigned int nr) > > +{ > > + int i; > > + struct page_ext *page_ext; > > + union codetag_ref *ref; > > + struct alloc_tag *tag; > > + > > + if (!mem_alloc_profiling_enabled()) > > + return; > > + > > + page_ext = page_ext_get(page); > > + if (unlikely(!page_ext)) > > + return; > > + > > + ref = codetag_ref_from_page_ext(page_ext); > > + if (!ref->ct) > > + goto out; > > + > > + tag = ct_to_alloc_tag(ref->ct); > > + page_ext = page_ext_next(page_ext); > > + for (i = 1; i < nr; i++) { > > + /* Set new reference to point to the original tag */ > > + alloc_tag_ref_set(codetag_ref_from_page_ext(page_ext), tag); > > + page_ext = page_ext_next(page_ext); > > + } > > +out: > > + page_ext_put(page_ext); > > +} > > + > > #else /* CONFIG_MEM_ALLOC_PROFILING */ > > > > static inline void pgalloc_tag_add(struct page *page, struct task_struct *task, > > unsigned int order) {} > > static inline void pgalloc_tag_sub(struct page *page, unsigned int order) {} > > +static inline void pgalloc_tag_split(struct page *page, unsigned int nr) {} > > > > #endif /* CONFIG_MEM_ALLOC_PROFILING */ > > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > index 94c958f7ebb5..86daae671319 100644 > > --- a/mm/huge_memory.c > > +++ b/mm/huge_memory.c > > @@ -38,6 +38,7 @@ > > #include <linux/sched/sysctl.h> > > #include <linux/memory-tiers.h> > > #include <linux/compat.h> > > +#include <linux/pgalloc_tag.h> > > > > #include <asm/tlb.h> > > #include <asm/pgalloc.h> > > @@ -2899,6 +2900,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, > > /* Caller disabled irqs, so they are still disabled here */ > > > > split_page_owner(head, nr); > > + pgalloc_tag_split(head, nr); > > > > /* See comment in __split_huge_page_tail() */ > > if (PageAnon(head)) { > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 58c0e8b948a4..4bc5b4720fee 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -2621,6 +2621,7 @@ void split_page(struct page *page, unsigned int order) > > for (i = 1; i < (1 << order); i++) > > set_page_refcounted(page + i); > > split_page_owner(page, 1 << order); > > + pgalloc_tag_split(page, 1 << order); > > split_page_memcg(page, 1 << order); > > } > > EXPORT_SYMBOL_GPL(split_page); > > @@ -4806,6 +4807,7 @@ static void *make_alloc_exact(unsigned long addr, unsigned int order, > > struct page *last = page + nr; > > > > split_page_owner(page, 1 << order); > > + pgalloc_tag_split(page, 1 << order); > > split_page_memcg(page, 1 << order); > > while (page < --last) > > set_page_refcounted(last);
On 2/27/24 17:38, Suren Baghdasaryan wrote: > On Tue, Feb 27, 2024 at 2:10 AM Vlastimil Babka <vbabka@suse.cz> wrote: >> >> On 2/21/24 20:40, Suren Baghdasaryan wrote: >> > When a high-order page is split into smaller ones, each newly split >> > page should get its codetag. The original codetag is reused for these >> > pages but it's recorded as 0-byte allocation because original codetag >> > already accounts for the original high-order allocated page. >> >> This was v3 but then you refactored (for the better) so the commit log >> could reflect it? > > Yes, technically mechnism didn't change but I should word it better. > Smth like this: > > When a high-order page is split into smaller ones, each newly split > page should get its codetag. After the split each split page will be > referencing the original codetag. The codetag's "bytes" counter > remains the same because the amount of allocated memory has not > changed, however the "calls" counter gets increased to keep the > counter correct when these individual pages get freed. Great, thanks. The concern with __free_pages() is not really related to splitting, so for this patch: Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > >> >> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> >> >> I was going to R-b, but now I recalled the trickiness of >> __free_pages() for non-compound pages if it loses the race to a >> speculative reference. Will the codetag handling work fine there? > > I think so. Each non-compoud page has its individual reference to its > codetag and will decrement it whenever the page is freed. IIUC the > logic in __free_pages(), when it loses race to a speculative > reference it will free all pages except for the first one and the The "tail" pages of this non-compound high-order page will AFAICS not have code tags assigned, so alloc_tag_sub() will be a no-op (or a warning with _DEBUG). > first one will be freed when the last put_page() happens. If prior to > this all these pages were split from one page then all of them will > have their own reference which points to the same codetag. Yeah I'm assuming there's no split before the freeing. This patch about splitting just reminded me of that tricky freeing scenario. So IIUC the "else if (!head)" path of __free_pages() will do nothing about the "tail" pages wrt code tags as there are no code tags. Then whoever took the speculative "head" page reference will put_page() and free it, which will end up in alloc_tag_sub(). This will decrement calls properly, but bytes will become imbalanced, because that put_page() will pass order-0 worth of bytes - the original order is lost. Now this might be rare enough that it's not worth fixing if that would be too complicated, just FYI. > Every time > one of these pages are freed that codetag's "bytes" and "calls" > counters will be decremented. I think accounting will work correctly > irrespective of where these pages are freed, in __free_pages() or by > put_page(). >
On Wed, Feb 28, 2024 at 12:47 AM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 2/27/24 17:38, Suren Baghdasaryan wrote: > > On Tue, Feb 27, 2024 at 2:10 AM Vlastimil Babka <vbabka@suse.cz> wrote: > >> > >> On 2/21/24 20:40, Suren Baghdasaryan wrote: > >> > When a high-order page is split into smaller ones, each newly split > >> > page should get its codetag. The original codetag is reused for these > >> > pages but it's recorded as 0-byte allocation because original codetag > >> > already accounts for the original high-order allocated page. > >> > >> This was v3 but then you refactored (for the better) so the commit log > >> could reflect it? > > > > Yes, technically mechnism didn't change but I should word it better. > > Smth like this: > > > > When a high-order page is split into smaller ones, each newly split > > page should get its codetag. After the split each split page will be > > referencing the original codetag. The codetag's "bytes" counter > > remains the same because the amount of allocated memory has not > > changed, however the "calls" counter gets increased to keep the > > counter correct when these individual pages get freed. > > Great, thanks. > The concern with __free_pages() is not really related to splitting, so for > this patch: > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > > > > >> > >> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > >> > >> I was going to R-b, but now I recalled the trickiness of > >> __free_pages() for non-compound pages if it loses the race to a > >> speculative reference. Will the codetag handling work fine there? > > > > I think so. Each non-compoud page has its individual reference to its > > codetag and will decrement it whenever the page is freed. IIUC the > > logic in __free_pages(), when it loses race to a speculative > > reference it will free all pages except for the first one and the > > The "tail" pages of this non-compound high-order page will AFAICS not have > code tags assigned, so alloc_tag_sub() will be a no-op (or a warning with > _DEBUG). Yes, that is correct. > > > first one will be freed when the last put_page() happens. If prior to > > this all these pages were split from one page then all of them will > > have their own reference which points to the same codetag. > > Yeah I'm assuming there's no split before the freeing. This patch about > splitting just reminded me of that tricky freeing scenario. Ah, I see. I thought you were talking about a page that was previously split. > > So IIUC the "else if (!head)" path of __free_pages() will do nothing about > the "tail" pages wrt code tags as there are no code tags. > Then whoever took the speculative "head" page reference will put_page() and > free it, which will end up in alloc_tag_sub(). This will decrement calls > properly, but bytes will become imbalanced, because that put_page() will > pass order-0 worth of bytes - the original order is lost. Yeah, that's true. put_page() will end up calling free_unref_page(&folio->page, 0) even if the original order was more than 0. > > Now this might be rare enough that it's not worth fixing if that would be > too complicated, just FYI. Yeah. We can fix this by subtracting the "bytes" counter of the "head" page for all free_the_page(page + (1 << order), order) calls we do inside __free_pages(). But we can't simply use pgalloc_tag_sub() because the "calls" counter will get over-decremented (we allocated all of these pages with one call). I'll need to introduce a new pgalloc_tag_sub_bytes() API and use it here. I feel it's too targeted of a solution but OTOH this is a special situation, so maybe it's acceptable. WDYT? > > > > Every time > > one of these pages are freed that codetag's "bytes" and "calls" > > counters will be decremented. I think accounting will work correctly > > irrespective of where these pages are freed, in __free_pages() or by > > put_page(). > > >
On 2/28/24 18:50, Suren Baghdasaryan wrote: > On Wed, Feb 28, 2024 at 12:47 AM Vlastimil Babka <vbabka@suse.cz> wrote: > >> >> Now this might be rare enough that it's not worth fixing if that would be >> too complicated, just FYI. > > Yeah. We can fix this by subtracting the "bytes" counter of the "head" > page for all free_the_page(page + (1 << order), order) calls we do > inside __free_pages(). But we can't simply use pgalloc_tag_sub() > because the "calls" counter will get over-decremented (we allocated > all of these pages with one call). I'll need to introduce a new > pgalloc_tag_sub_bytes() API and use it here. I feel it's too targeted > of a solution but OTOH this is a special situation, so maybe it's > acceptable. WDYT? Hmm I think there's a problem that once you fail put_page_testzero() and detect you need to do this, the page might be already gone or reallocated so you can't get to the tag for decrementing bytes. You'd have to get it upfront (I guess for "head && order > 0" cases) just in case it happens. Maybe it's not worth the trouble for such a rare case. >> >> >> > Every time >> > one of these pages are freed that codetag's "bytes" and "calls" >> > counters will be decremented. I think accounting will work correctly >> > irrespective of where these pages are freed, in __free_pages() or by >> > put_page(). >> > >>
On Wed, Feb 28, 2024 at 10:28 AM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 2/28/24 18:50, Suren Baghdasaryan wrote: > > On Wed, Feb 28, 2024 at 12:47 AM Vlastimil Babka <vbabka@suse.cz> wrote: > > > >> > >> Now this might be rare enough that it's not worth fixing if that would be > >> too complicated, just FYI. > > > > Yeah. We can fix this by subtracting the "bytes" counter of the "head" > > page for all free_the_page(page + (1 << order), order) calls we do > > inside __free_pages(). But we can't simply use pgalloc_tag_sub() > > because the "calls" counter will get over-decremented (we allocated > > all of these pages with one call). I'll need to introduce a new > > pgalloc_tag_sub_bytes() API and use it here. I feel it's too targeted > > of a solution but OTOH this is a special situation, so maybe it's > > acceptable. WDYT? > > Hmm I think there's a problem that once you fail put_page_testzero() and > detect you need to do this, the page might be already gone or reallocated so > you can't get to the tag for decrementing bytes. You'd have to get it > upfront (I guess for "head && order > 0" cases) just in case it happens. > Maybe it's not worth the trouble for such a rare case. Yes, that hit me when I tried to implement it but there is a simple solution around that. I can obtain alloc_tag before doing put_page_testzero() and then decrement bytes counter directly as needed. Not sure if it is a rare enough case that we can ignore it but if the fix is simple enough then might as well do it? > > >> > >> > >> > Every time > >> > one of these pages are freed that codetag's "bytes" and "calls" > >> > counters will be decremented. I think accounting will work correctly > >> > irrespective of where these pages are freed, in __free_pages() or by > >> > put_page(). > >> > > >> > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
diff --git a/include/linux/pgalloc_tag.h b/include/linux/pgalloc_tag.h index b49ab955300f..9e6ad8e0e4aa 100644 --- a/include/linux/pgalloc_tag.h +++ b/include/linux/pgalloc_tag.h @@ -67,11 +67,41 @@ static inline void pgalloc_tag_sub(struct page *page, unsigned int order) } } +static inline void pgalloc_tag_split(struct page *page, unsigned int nr) +{ + int i; + struct page_ext *page_ext; + union codetag_ref *ref; + struct alloc_tag *tag; + + if (!mem_alloc_profiling_enabled()) + return; + + page_ext = page_ext_get(page); + if (unlikely(!page_ext)) + return; + + ref = codetag_ref_from_page_ext(page_ext); + if (!ref->ct) + goto out; + + tag = ct_to_alloc_tag(ref->ct); + page_ext = page_ext_next(page_ext); + for (i = 1; i < nr; i++) { + /* Set new reference to point to the original tag */ + alloc_tag_ref_set(codetag_ref_from_page_ext(page_ext), tag); + page_ext = page_ext_next(page_ext); + } +out: + page_ext_put(page_ext); +} + #else /* CONFIG_MEM_ALLOC_PROFILING */ static inline void pgalloc_tag_add(struct page *page, struct task_struct *task, unsigned int order) {} static inline void pgalloc_tag_sub(struct page *page, unsigned int order) {} +static inline void pgalloc_tag_split(struct page *page, unsigned int nr) {} #endif /* CONFIG_MEM_ALLOC_PROFILING */ diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 94c958f7ebb5..86daae671319 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -38,6 +38,7 @@ #include <linux/sched/sysctl.h> #include <linux/memory-tiers.h> #include <linux/compat.h> +#include <linux/pgalloc_tag.h> #include <asm/tlb.h> #include <asm/pgalloc.h> @@ -2899,6 +2900,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, /* Caller disabled irqs, so they are still disabled here */ split_page_owner(head, nr); + pgalloc_tag_split(head, nr); /* See comment in __split_huge_page_tail() */ if (PageAnon(head)) { diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 58c0e8b948a4..4bc5b4720fee 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2621,6 +2621,7 @@ void split_page(struct page *page, unsigned int order) for (i = 1; i < (1 << order); i++) set_page_refcounted(page + i); split_page_owner(page, 1 << order); + pgalloc_tag_split(page, 1 << order); split_page_memcg(page, 1 << order); } EXPORT_SYMBOL_GPL(split_page); @@ -4806,6 +4807,7 @@ static void *make_alloc_exact(unsigned long addr, unsigned int order, struct page *last = page + nr; split_page_owner(page, 1 << order); + pgalloc_tag_split(page, 1 << order); split_page_memcg(page, 1 << order); while (page < --last) set_page_refcounted(last);
When a high-order page is split into smaller ones, each newly split page should get its codetag. The original codetag is reused for these pages but it's recorded as 0-byte allocation because original codetag already accounts for the original high-order allocated page. Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- include/linux/pgalloc_tag.h | 30 ++++++++++++++++++++++++++++++ mm/huge_memory.c | 2 ++ mm/page_alloc.c | 2 ++ 3 files changed, 34 insertions(+)