On Tue, Mar 14, 2023 at 8:33 PM Waiman Long <longman@xxxxxxxxxx> wrote: > > On 3/14/23 23:10, Yosry Ahmed wrote: > >>> The only other user today is print_page_owner_memcg(). I am not sure > >>> if it's doing the right thing by explicitly reading page->memcg_data, > >>> but it is already excluding pages that have page->memcg_data == 0, > >>> which should be the case for tail pages. > >> It is reading memcg_data directly to see if it is slab cache page. It is > >> currently skipping page that does not have memcg_data set. > > IIUC this skips tail pages, because they should always have > > page->memcg_data == 0, even if they are charged to a memcg. To > > correctly get their memcg we should read it from the > > compound_head()/page_folio(). > The purpose of that function is mainly to report pages that have a > reference to a memcg, especially the dead one. So by counting the > occurrence of a particular cgroup name, we can have a rough idea of > that. So only head page has relevance here and we can skip the tail pages. I see. If you update the code to not check memcg_data directly, and we update page_memcg_check() to return memcg of the head page, this will stop skipping tail pages. Is this okay? If not, we can explicitly skip tail pages in print_page_owner_memcg() if we decide to check the head's memcg in page_memcg_check(). > > > > My 2c, we can check PageSlab() to print the extra message for slab > > pages, instead of reading memcg_data directly, which kinda breaks the > > abstraction created by the various helpers for reading a page memcg. > > Someone can easily change something in how memcg_data is interpreted > > in those helpers without realizing that page_owner is also reading it. > > You are right. We should be using a helper if available. I will send a > patch to fix that. Great, thanks! > > Thanks, > Longman >
