| Message ID | 20231213130414.353244-1-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: memcg: remove direct use of __memcg_kmem_uncharge_page | expand |
> On Dec 13, 2023, at 21:04, Yosry Ahmed <yosryahmed@google.com> wrote: > > memcg_kmem_uncharge_page() is an inline wrapper around > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before > making the function call. Internally, __memcg_kmem_uncharge_page() has a > folio_memcg_kmem() check. > > The only direct user of __memcg_kmem_uncharge_page(), > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid > the function call if possible. Move the folio_memcg_kmem() check from > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as > PageMemcgKmem() -- which does the same thing under the hood. Now > free_pages_prepare() can also use memcg_kmem_uncharge_page(). > > No functional change intended. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com> Thanks
On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote: > memcg_kmem_uncharge_page() is an inline wrapper around > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before > making the function call. Internally, __memcg_kmem_uncharge_page() has a > folio_memcg_kmem() check. > > The only direct user of __memcg_kmem_uncharge_page(), > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid > the function call if possible. Move the folio_memcg_kmem() check from > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as > PageMemcgKmem() -- which does the same thing under the hood. Now > free_pages_prepare() can also use memcg_kmem_uncharge_page(). I think you've just pessimised all the other places which call memcg_kmem_uncharge_page(). It's a matter of probabilities. In free_pages_prepare(), most of the pages being freed are not accounted to memcg. Whereas in fork() we are absolutely certain that the pages were accounted because we accounted them. I think this is a bad change.
On Wed, Dec 13, 2023 at 7:01 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote: > > memcg_kmem_uncharge_page() is an inline wrapper around > > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before > > making the function call. Internally, __memcg_kmem_uncharge_page() has a > > folio_memcg_kmem() check. > > > > The only direct user of __memcg_kmem_uncharge_page(), > > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid > > the function call if possible. Move the folio_memcg_kmem() check from > > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as > > PageMemcgKmem() -- which does the same thing under the hood. Now > > free_pages_prepare() can also use memcg_kmem_uncharge_page(). > > I think you've just pessimised all the other places which call > memcg_kmem_uncharge_page(). It's a matter of probabilities. In > free_pages_prepare(), most of the pages being freed are not accounted > to memcg. Whereas in fork() we are absolutely certain that the pages > were accounted because we accounted them. The check was already there for other callers, but it was inside __memcg_kmem_uncharge_page(). IIUC, the only change for other callers is an extra call to compound_head(), and they are not hot paths AFAICT so it shouldn't be noticeable. Am I missing something? Perhaps your point is about how branch prediction works across function call boundaries? or is this not about performance at all? > > I think this is a bad change.
On Wed, Dec 13, 2023 at 07:08:52AM -0800, Yosry Ahmed wrote: > On Wed, Dec 13, 2023 at 7:01 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote: > > > memcg_kmem_uncharge_page() is an inline wrapper around > > > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before > > > making the function call. Internally, __memcg_kmem_uncharge_page() has a > > > folio_memcg_kmem() check. > > > > > > The only direct user of __memcg_kmem_uncharge_page(), > > > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid > > > the function call if possible. Move the folio_memcg_kmem() check from > > > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as > > > PageMemcgKmem() -- which does the same thing under the hood. Now > > > free_pages_prepare() can also use memcg_kmem_uncharge_page(). > > > > I think you've just pessimised all the other places which call > > memcg_kmem_uncharge_page(). It's a matter of probabilities. In > > free_pages_prepare(), most of the pages being freed are not accounted > > to memcg. Whereas in fork() we are absolutely certain that the pages > > were accounted because we accounted them. > > The check was already there for other callers, but it was inside > __memcg_kmem_uncharge_page(). IIUC, the only change for other callers > is an extra call to compound_head(), and they are not hot paths AFAICT > so it shouldn't be noticeable. How can you seriously claim that fork() is not a hot path? > Am I missing something? Perhaps your point is about how branch > prediction works across function call boundaries? or is this not about > performance at all? > > > > > I think this is a bad change.
On Wed, Dec 13, 2023 at 7:38 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Dec 13, 2023 at 07:08:52AM -0800, Yosry Ahmed wrote: > > On Wed, Dec 13, 2023 at 7:01 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote: > > > > memcg_kmem_uncharge_page() is an inline wrapper around > > > > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before > > > > making the function call. Internally, __memcg_kmem_uncharge_page() has a > > > > folio_memcg_kmem() check. > > > > > > > > The only direct user of __memcg_kmem_uncharge_page(), > > > > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid > > > > the function call if possible. Move the folio_memcg_kmem() check from > > > > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as > > > > PageMemcgKmem() -- which does the same thing under the hood. Now > > > > free_pages_prepare() can also use memcg_kmem_uncharge_page(). > > > > > > I think you've just pessimised all the other places which call > > > memcg_kmem_uncharge_page(). It's a matter of probabilities. In > > > free_pages_prepare(), most of the pages being freed are not accounted > > > to memcg. Whereas in fork() we are absolutely certain that the pages > > > were accounted because we accounted them. > > > > The check was already there for other callers, but it was inside > > __memcg_kmem_uncharge_page(). IIUC, the only change for other callers > > is an extra call to compound_head(), and they are not hot paths AFAICT > > so it shouldn't be noticeable. > > How can you seriously claim that fork() is not a hot path? It's only called in fork() when an error happens. It's normally called when a process is exiting.
On Wed, Dec 13, 2023 at 07:42:44AM -0800, Yosry Ahmed wrote: > On Wed, Dec 13, 2023 at 7:38 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Wed, Dec 13, 2023 at 07:08:52AM -0800, Yosry Ahmed wrote: > > > On Wed, Dec 13, 2023 at 7:01 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote: > > > > > memcg_kmem_uncharge_page() is an inline wrapper around > > > > > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before > > > > > making the function call. Internally, __memcg_kmem_uncharge_page() has a > > > > > folio_memcg_kmem() check. > > > > > > > > > > The only direct user of __memcg_kmem_uncharge_page(), > > > > > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid > > > > > the function call if possible. Move the folio_memcg_kmem() check from > > > > > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as > > > > > PageMemcgKmem() -- which does the same thing under the hood. Now > > > > > free_pages_prepare() can also use memcg_kmem_uncharge_page(). > > > > > > > > I think you've just pessimised all the other places which call > > > > memcg_kmem_uncharge_page(). It's a matter of probabilities. In > > > > free_pages_prepare(), most of the pages being freed are not accounted > > > > to memcg. Whereas in fork() we are absolutely certain that the pages > > > > were accounted because we accounted them. > > > > > > The check was already there for other callers, but it was inside > > > __memcg_kmem_uncharge_page(). IIUC, the only change for other callers > > > is an extra call to compound_head(), and they are not hot paths AFAICT > > > so it shouldn't be noticeable. > > > > How can you seriously claim that fork() is not a hot path? > > It's only called in fork() when an error happens. It's normally called > when a process is exiting. process exit is also a hot path. at least, there have been regressions reported that it's "too slow".
On Wed, Dec 13, 2023 at 8:23 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Dec 13, 2023 at 07:42:44AM -0800, Yosry Ahmed wrote: > > On Wed, Dec 13, 2023 at 7:38 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Wed, Dec 13, 2023 at 07:08:52AM -0800, Yosry Ahmed wrote: > > > > On Wed, Dec 13, 2023 at 7:01 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > > > On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote: > > > > > > memcg_kmem_uncharge_page() is an inline wrapper around > > > > > > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before > > > > > > making the function call. Internally, __memcg_kmem_uncharge_page() has a > > > > > > folio_memcg_kmem() check. > > > > > > > > > > > > The only direct user of __memcg_kmem_uncharge_page(), > > > > > > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid > > > > > > the function call if possible. Move the folio_memcg_kmem() check from > > > > > > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as > > > > > > PageMemcgKmem() -- which does the same thing under the hood. Now > > > > > > free_pages_prepare() can also use memcg_kmem_uncharge_page(). > > > > > > > > > > I think you've just pessimised all the other places which call > > > > > memcg_kmem_uncharge_page(). It's a matter of probabilities. In > > > > > free_pages_prepare(), most of the pages being freed are not accounted > > > > > to memcg. Whereas in fork() we are absolutely certain that the pages > > > > > were accounted because we accounted them. > > > > > > > > The check was already there for other callers, but it was inside > > > > __memcg_kmem_uncharge_page(). IIUC, the only change for other callers > > > > is an extra call to compound_head(), and they are not hot paths AFAICT > > > > so it shouldn't be noticeable. > > > > > > How can you seriously claim that fork() is not a hot path? > > > > It's only called in fork() when an error happens. It's normally called > > when a process is exiting. > > process exit is also a hot path. at least, there have been regressions > reported that it's "too slow". I doubt an extra compound_head() will matter in that path, but if you feel strongly about it that's okay. It's a nice cleanup that's all.
On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > I doubt an extra compound_head() will matter in that path, but if you > feel strongly about it that's okay. It's a nice cleanup that's all. i don't even understand why you think it's a nice cleanup.
On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > > I doubt an extra compound_head() will matter in that path, but if you > > feel strongly about it that's okay. It's a nice cleanup that's all. > > i don't even understand why you think it's a nice cleanup. free_pages_prepare() is directly calling __memcg_kmem_uncharge_page() instead of memcg_kmem_uncharge_page(), and open-coding checks that already exist in both of them to avoid the unnecessary function call if possible. I think this should be the job of memcg_kmem_uncharge_page(), but it's currently missing the PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()). So I think moving that check to the wrapper allows free_pages_prepare() to call memcg_kmem_uncharge_page() and without worrying about those memcg-specific checks.
On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote: > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > > > I doubt an extra compound_head() will matter in that path, but if you > > > feel strongly about it that's okay. It's a nice cleanup that's all. > > > > i don't even understand why you think it's a nice cleanup. > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page() > instead of memcg_kmem_uncharge_page(), and open-coding checks that > already exist in both of them to avoid the unnecessary function call > if possible. I think this should be the job of > memcg_kmem_uncharge_page(), but it's currently missing the > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()). > > So I think moving that check to the wrapper allows > free_pages_prepare() to call memcg_kmem_uncharge_page() and without > worrying about those memcg-specific checks. There is a (performance) reason these open coded check are present in page_alloc.c and that is very clear for __memcg_kmem_charge_page() but not so much for __memcg_kmem_uncharge_page(). So, for uncharge path, this seems ok. Now to resolve Willy's concern for the fork() path, I think we can open code the checks there. Willy, any concern with that approach?
On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote: > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote: > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > > > > I doubt an extra compound_head() will matter in that path, but if you > > > > feel strongly about it that's okay. It's a nice cleanup that's all. > > > > > > i don't even understand why you think it's a nice cleanup. > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page() > > instead of memcg_kmem_uncharge_page(), and open-coding checks that > > already exist in both of them to avoid the unnecessary function call > > if possible. I think this should be the job of > > memcg_kmem_uncharge_page(), but it's currently missing the > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()). > > > > So I think moving that check to the wrapper allows > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without > > worrying about those memcg-specific checks. > > There is a (performance) reason these open coded check are present in > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path, > this seems ok. Now to resolve Willy's concern for the fork() path, I > think we can open code the checks there. > > Willy, any concern with that approach? The justification for this change is insufficient. Or really any change in this area. It's fine the way it is. "The check is done twice" is really weak, when the check is so cheap (much cheaper than calling compound_head!)
On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote: > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote: > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > > > > > I doubt an extra compound_head() will matter in that path, but if you > > > > > feel strongly about it that's okay. It's a nice cleanup that's all. > > > > > > > > i don't even understand why you think it's a nice cleanup. > > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page() > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that > > > already exist in both of them to avoid the unnecessary function call > > > if possible. I think this should be the job of > > > memcg_kmem_uncharge_page(), but it's currently missing the > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()). > > > > > > So I think moving that check to the wrapper allows > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without > > > worrying about those memcg-specific checks. > > > > There is a (performance) reason these open coded check are present in > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path, > > this seems ok. Now to resolve Willy's concern for the fork() path, I > > think we can open code the checks there. > > > > Willy, any concern with that approach? > > The justification for this change is insufficient. Or really any change > in this area. It's fine the way it is. "The check is done twice" is > really weak, when the check is so cheap (much cheaper than calling > compound_head!) I think that is what Yosry is trying i.e. reducing two calls to page_folio() to one in the page free path.
On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote: > > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote: > > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > > > > > > I doubt an extra compound_head() will matter in that path, but if you > > > > > > feel strongly about it that's okay. It's a nice cleanup that's all. > > > > > > > > > > i don't even understand why you think it's a nice cleanup. > > > > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page() > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that > > > > already exist in both of them to avoid the unnecessary function call > > > > if possible. I think this should be the job of > > > > memcg_kmem_uncharge_page(), but it's currently missing the > > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()). > > > > > > > > So I think moving that check to the wrapper allows > > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without > > > > worrying about those memcg-specific checks. > > > > > > There is a (performance) reason these open coded check are present in > > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but > > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path, > > > this seems ok. Now to resolve Willy's concern for the fork() path, I > > > think we can open code the checks there. > > > > > > Willy, any concern with that approach? > > > > The justification for this change is insufficient. Or really any change > > in this area. It's fine the way it is. "The check is done twice" is > > really weak, when the check is so cheap (much cheaper than calling > > compound_head!) > > I think that is what Yosry is trying i.e. reducing two calls to > page_folio() to one in the page free path. Actually no, there will still be two calls to page_folio() even after Yosry's change. One for PageMemcgKmem() and second in __memcg_kmem_uncharge_page(). I think I agree with Willy that this patch is actually adding one more page_folio() call to the fork code path. Maybe we just need to remove PageMemcgKmem() check in the free_pages_prepare() and that's all.
On Wed, Dec 13, 2023 at 12:58 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote: > > > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote: > > > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > > > > > > > I doubt an extra compound_head() will matter in that path, but if you > > > > > > > feel strongly about it that's okay. It's a nice cleanup that's all. > > > > > > > > > > > > i don't even understand why you think it's a nice cleanup. > > > > > > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page() > > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that > > > > > already exist in both of them to avoid the unnecessary function call > > > > > if possible. I think this should be the job of > > > > > memcg_kmem_uncharge_page(), but it's currently missing the > > > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()). > > > > > > > > > > So I think moving that check to the wrapper allows > > > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without > > > > > worrying about those memcg-specific checks. > > > > > > > > There is a (performance) reason these open coded check are present in > > > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but > > > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path, > > > > this seems ok. Now to resolve Willy's concern for the fork() path, I > > > > think we can open code the checks there. > > > > > > > > Willy, any concern with that approach? > > > > > > The justification for this change is insufficient. Or really any change > > > in this area. It's fine the way it is. "The check is done twice" is > > > really weak, when the check is so cheap (much cheaper than calling > > > compound_head!) > > > > I think that is what Yosry is trying i.e. reducing two calls to > > page_folio() to one in the page free path. > > Actually no, there will still be two calls to page_folio() even after > Yosry's change. One for PageMemcgKmem() and second in > __memcg_kmem_uncharge_page(). > > I think I agree with Willy that this patch is actually adding one more > page_folio() call to the fork code path. It is adding one more page_folio(), yes, but to the process exit path. > > Maybe we just need to remove PageMemcgKmem() check in the > free_pages_prepare() and that's all. You mean call memcg_kmem_charge_page() directly in free_pages_prepare() without the PageMemcgKmem()? I think we can do that. My understanding is that this is not the case today because we want to avoid the function call if !PageMemcgKmem(). Do you believe the cost of the function call is negligible?
On Wed, Dec 13, 2023 at 2:05 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > On Wed, Dec 13, 2023 at 12:58 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > > > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote: > > > > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote: > > > > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > > > > > > > > I doubt an extra compound_head() will matter in that path, but if you > > > > > > > > feel strongly about it that's okay. It's a nice cleanup that's all. > > > > > > > > > > > > > > i don't even understand why you think it's a nice cleanup. > > > > > > > > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page() > > > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that > > > > > > already exist in both of them to avoid the unnecessary function call > > > > > > if possible. I think this should be the job of > > > > > > memcg_kmem_uncharge_page(), but it's currently missing the > > > > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()). > > > > > > > > > > > > So I think moving that check to the wrapper allows > > > > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without > > > > > > worrying about those memcg-specific checks. > > > > > > > > > > There is a (performance) reason these open coded check are present in > > > > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but > > > > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path, > > > > > this seems ok. Now to resolve Willy's concern for the fork() path, I > > > > > think we can open code the checks there. > > > > > > > > > > Willy, any concern with that approach? > > > > > > > > The justification for this change is insufficient. Or really any change > > > > in this area. It's fine the way it is. "The check is done twice" is > > > > really weak, when the check is so cheap (much cheaper than calling > > > > compound_head!) > > > > > > I think that is what Yosry is trying i.e. reducing two calls to > > > page_folio() to one in the page free path. > > > > Actually no, there will still be two calls to page_folio() even after > > Yosry's change. One for PageMemcgKmem() and second in > > __memcg_kmem_uncharge_page(). > > > > I think I agree with Willy that this patch is actually adding one more > > page_folio() call to the fork code path. > > It is adding one more page_folio(), yes, but to the process exit path. > > > > > Maybe we just need to remove PageMemcgKmem() check in the > > free_pages_prepare() and that's all. > > You mean call memcg_kmem_charge_page() directly in > free_pages_prepare() without the PageMemcgKmem()? I think we can do > that. My understanding is that this is not the case today because we > want to avoid the function call if !PageMemcgKmem(). Do you believe > the cost of the function call is negligible? The compiler can potentially inline that function but on the other hand we will do twice reads of page->compound_head due to READ_ONCE(). We don't have data to support one option or the other. Unless we can show perf difference between the two, I think doing nothing (leave it as is) will be the better use of our time.
On Wed, Dec 13, 2023 at 2:12 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Wed, Dec 13, 2023 at 2:05 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > On Wed, Dec 13, 2023 at 12:58 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > > > On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > > > > > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote: > > > > > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote: > > > > > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > > > > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > > > > > > > > > I doubt an extra compound_head() will matter in that path, but if you > > > > > > > > > feel strongly about it that's okay. It's a nice cleanup that's all. > > > > > > > > > > > > > > > > i don't even understand why you think it's a nice cleanup. > > > > > > > > > > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page() > > > > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that > > > > > > > already exist in both of them to avoid the unnecessary function call > > > > > > > if possible. I think this should be the job of > > > > > > > memcg_kmem_uncharge_page(), but it's currently missing the > > > > > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()). > > > > > > > > > > > > > > So I think moving that check to the wrapper allows > > > > > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without > > > > > > > worrying about those memcg-specific checks. > > > > > > > > > > > > There is a (performance) reason these open coded check are present in > > > > > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but > > > > > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path, > > > > > > this seems ok. Now to resolve Willy's concern for the fork() path, I > > > > > > think we can open code the checks there. > > > > > > > > > > > > Willy, any concern with that approach? > > > > > > > > > > The justification for this change is insufficient. Or really any change > > > > > in this area. It's fine the way it is. "The check is done twice" is > > > > > really weak, when the check is so cheap (much cheaper than calling > > > > > compound_head!) > > > > > > > > I think that is what Yosry is trying i.e. reducing two calls to > > > > page_folio() to one in the page free path. > > > > > > Actually no, there will still be two calls to page_folio() even after > > > Yosry's change. One for PageMemcgKmem() and second in > > > __memcg_kmem_uncharge_page(). > > > > > > I think I agree with Willy that this patch is actually adding one more > > > page_folio() call to the fork code path. > > > > It is adding one more page_folio(), yes, but to the process exit path. > > > > > > > > Maybe we just need to remove PageMemcgKmem() check in the > > > free_pages_prepare() and that's all. > > > > You mean call memcg_kmem_charge_page() directly in > > free_pages_prepare() without the PageMemcgKmem()? I think we can do > > that. My understanding is that this is not the case today because we > > want to avoid the function call if !PageMemcgKmem(). Do you believe > > the cost of the function call is negligible? > > The compiler can potentially inline that function but on the other > hand we will do twice reads of page->compound_head due to READ_ONCE(). > > We don't have data to support one option or the other. Unless we can > show perf difference between the two, I think doing nothing (leave it > as is) will be the better use of our time. Ack, let's just leave it for now. FWIW, I believe what this patch is doing will eventually be the right thing to do once the code is folio-ized and the calls to page_folio() disappear naturally anyway.
On Wed, Dec 13, 2023 at 02:12:35PM -0800, Shakeel Butt wrote: > On Wed, Dec 13, 2023 at 2:05 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > On Wed, Dec 13, 2023 at 12:58 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > > > On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > > > > > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote: > > > > > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote: > > > > > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > > > > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > > > > > > > > > I doubt an extra compound_head() will matter in that path, but if you > > > > > > > > > feel strongly about it that's okay. It's a nice cleanup that's all. > > > > > > > > > > > > > > > > i don't even understand why you think it's a nice cleanup. > > > > > > > > > > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page() > > > > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that > > > > > > > already exist in both of them to avoid the unnecessary function call > > > > > > > if possible. I think this should be the job of > > > > > > > memcg_kmem_uncharge_page(), but it's currently missing the > > > > > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()). > > > > > > > > > > > > > > So I think moving that check to the wrapper allows > > > > > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without > > > > > > > worrying about those memcg-specific checks. > > > > > > > > > > > > There is a (performance) reason these open coded check are present in > > > > > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but > > > > > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path, > > > > > > this seems ok. Now to resolve Willy's concern for the fork() path, I > > > > > > think we can open code the checks there. > > > > > > > > > > > > Willy, any concern with that approach? > > > > > > > > > > The justification for this change is insufficient. Or really any change > > > > > in this area. It's fine the way it is. "The check is done twice" is > > > > > really weak, when the check is so cheap (much cheaper than calling > > > > > compound_head!) > > > > > > > > I think that is what Yosry is trying i.e. reducing two calls to > > > > page_folio() to one in the page free path. > > > > > > Actually no, there will still be two calls to page_folio() even after > > > Yosry's change. One for PageMemcgKmem() and second in > > > __memcg_kmem_uncharge_page(). > > > > > > I think I agree with Willy that this patch is actually adding one more > > > page_folio() call to the fork code path. > > > > It is adding one more page_folio(), yes, but to the process exit path. > > > > > > > > Maybe we just need to remove PageMemcgKmem() check in the > > > free_pages_prepare() and that's all. > > > > You mean call memcg_kmem_charge_page() directly in > > free_pages_prepare() without the PageMemcgKmem()? I think we can do > > that. My understanding is that this is not the case today because we > > want to avoid the function call if !PageMemcgKmem(). Do you believe > > the cost of the function call is negligible? > > The compiler can potentially inline that function but on the other > hand we will do twice reads of page->compound_head due to READ_ONCE(). > > We don't have data to support one option or the other. Unless we can > show perf difference between the two, I think doing nothing (leave it > as is) will be the better use of our time. +1, especially given how controversial the change is.
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index a308c8eacf20d..2009ca508271a 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -1844,7 +1844,7 @@ static inline int memcg_kmem_charge_page(struct page *page, gfp_t gfp, static inline void memcg_kmem_uncharge_page(struct page *page, int order) { - if (memcg_kmem_online()) + if (memcg_kmem_online() && PageMemcgKmem(page)) __memcg_kmem_uncharge_page(page, order); } diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 69b0ad4552425..09efbfa2733e8 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3301,9 +3301,6 @@ void __memcg_kmem_uncharge_page(struct page *page, int order) struct obj_cgroup *objcg; unsigned int nr_pages = 1 << order; - if (!folio_memcg_kmem(folio)) - return; - objcg = __folio_objcg(folio); obj_cgroup_uncharge_pages(objcg, nr_pages); folio->memcg_data = 0; diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 7ea9c33320bf1..f72693d91ac22 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -1086,8 +1086,7 @@ static __always_inline bool free_pages_prepare(struct page *page, trace_mm_page_free(page, order); kmsan_free_page(page, order); - if (memcg_kmem_online() && PageMemcgKmem(page)) - __memcg_kmem_uncharge_page(page, order); + memcg_kmem_uncharge_page(page, order); if (unlikely(PageHWPoison(page)) && !order) { /* Do not let hwpoison pages hit pcplists/buddy */
memcg_kmem_uncharge_page() is an inline wrapper around __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before making the function call. Internally, __memcg_kmem_uncharge_page() has a folio_memcg_kmem() check. The only direct user of __memcg_kmem_uncharge_page(), free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid the function call if possible. Move the folio_memcg_kmem() check from __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as PageMemcgKmem() -- which does the same thing under the hood. Now free_pages_prepare() can also use memcg_kmem_uncharge_page(). No functional change intended. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- include/linux/memcontrol.h | 2 +- mm/memcontrol.c | 3 --- mm/page_alloc.c | 3 +-- 3 files changed, 2 insertions(+), 6 deletions(-)