Hi Johannes, On Thu, Feb 20, 2020 at 1:03 PM Johannes Weiner <hannes@xxxxxxxxxxx> wrote: > > Hey Shakeel! > > On Thu, Feb 20, 2020 at 10:14:45AM -0800, Shakeel Butt wrote: > > On Thu, Feb 20, 2020 at 8:52 AM Dan Schatzberg <schatzberg.dan@xxxxxxxxx> wrote: > > > > > > memalloc_use_memcg() worked for kernel allocations but was silently > > > ignored for user pages. > > > > > > This patch establishes a precedence order for who gets charged: > > > > > > 1. If there is a memcg associated with the page already, that memcg is > > > charged. This happens during swapin. > > > > > > 2. If an explicit mm is passed, mm->memcg is charged. This happens > > > during page faults, which can be triggered in remote VMs (eg gup). > > > > > > 3. Otherwise consult the current process context. If it has configured > > > a current->active_memcg, use that. > > > > What if css_tryget_online(current->active_memcg) in > > get_mem_cgroup_from_current() fails? Do we want to change this to > > css_tryget() and even if that fails should we fallback to > > root_mem_cgroup or current->mm->memcg? > > Good questions. > > I think we can switch to css_tryget(). If a cgroup goes offline > between issuing the IO and the loop layer executing that IO, the > resources used could end up in the root instead of the closest > ancestor of the offlined group. However, the risk of that actually > happening and causing problems is probably pretty small, and the > behavior isn't really worse than before Dan's patches. Agreed. > > Would you mind sending a separate patch for this? AFAICS similar > concerns apply to all users of foreign charging. Sure and yes similar concerns apply to other users as well. > > As for tryget failing: can that actually happen? AFAICS, all current > users acquire a reference first (get_memcg_from_somewhere()) that they > assign to current->active_memcg. We should probably codify this rule > and do WARN_ON(!css_tryget()) /* current->active_memcg must hold a ref */ Yes, we should WARN_ON(). Shakeel
