Messages in this thread | | | Subject | Re: [PATCH] mm/memcg: fix device private memcg accounting | From | Ralph Campbell <> | Date | Mon, 12 Oct 2020 10:11:07 -0700 |
| |
On 10/12/20 6:28 AM, Johannes Weiner wrote: > On Fri, Oct 09, 2020 at 05:00:37PM -0700, Ralph Campbell wrote: >> >> On 10/9/20 3:50 PM, Andrew Morton wrote: >>> On Fri, 9 Oct 2020 14:59:52 -0700 Ralph Campbell <rcampbell@nvidia.com> wrote: >>> >>>> The code in mc_handle_swap_pte() checks for non_swap_entry() and returns >>>> NULL before checking is_device_private_entry() so device private pages >>>> are never handled. >>>> Fix this by checking for non_swap_entry() after handling device private >>>> swap PTEs. > > The fix looks good to me. > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > >>> But this makes me suspect the answer is "there aren't any that we know >>> of". Are you sure a cc:stable is warranted? >>> >> >> I assume the memory cgroup accounting would be off somehow when moving >> a process to another memory cgroup. >> Currently, the device private page is charged like a normal anonymous page >> when allocated and is uncharged when the page is freed so I think that path is OK. >> Maybe someone who knows more about memory cgroup accounting can comment? > > As for whether to CC stable, I'm leaning toward no: > > - When moving tasks, we'd leave their device pages behind in the old > cgroup. This isn't great, but it doesn't cause counter imbalances or > corruption or anything - we also skip locked pages, we used to skip > pages mapped by more than one pte, the user can select whether to > move pages along tasks at all, and if so, whether only anon or file. > > - Charge moving itself is a bit of a questionable feature, and users > have been moving away from it. Leaving tasks in a cgroup and > changing the configuration is a heck of a lot cheaper than moving > potentially gigabytes of pages to another configuration domain. > > - According to the Fixes tag, this isn't a regression, either. Since > their inception, we have never migrated device pages.
Thanks for the Acked-by and the comments. I assume Andrew will update the tags when queuing this up unless he wants me to resend.
| |