On Wed, Feb 28, 2024 at 12:47 AM Vlastimil Babka <vbabka@xxxxxxx> wrote: > > On 2/27/24 17:38, Suren Baghdasaryan wrote: > > On Tue, Feb 27, 2024 at 2:10 AM Vlastimil Babka <vbabka@xxxxxxx> 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@xxxxxxx> > > > > >> > >> > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx> > >> > >> 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(). > > >
