Re: [PATCH] mm: net: memcg accounting for TCP rx zerocopy

On Wed, Jan 13, 2021 at 10:48 AM Roman Gushchin [off-list ref] wrote:

[snip]

quoted

quoted

Does it mean that a page can be accounted twice (even temporarily)?

This was an actual consideration for this patchset that we went back
and forth on a little bit.
Short answer, for this patch in its current form: yes. We're calling
mem_cgroup_charge_sock_pages() immediately prior to vm_insert_pages();
and the skb isn't cleaned up until afterwards. Thus we have a period
of double charging.

The pseudocode for the approach in this patch is:

while skb = next skb in queue is not null:
    charge_skb_pages(skb.pages) // sets page.memcg for each page
    // at this point pages are double counted
    vm_insert_pages(skb.pages)
    free(skb) // unrefs the pages, no longer double counted

An alternative version of this patch went the other way: have a short
period of undercharging.

while skb = next skb in queue is not null:
    for page in skb.pages set page.memcg
    vm_insert_pages(skb.pages)
    free(skb) // unrefs the pages. pages are now undercounted
charge_skb_pages(nr_pages_mapped, FORCE_CHARGE) // count is now correct again
ret

I have to think more, but at the first look the second approach is better.
IMO forcing the charge is less of a problem than double accounting
(we're forcing sock memory charging anyway). I'm afraid that even if the
double counting is temporarily for each individual page, with a constant
traffic it will create a permanent difference.

The double accounting behavior is a bit different in cgroup v1 vs v2
world as skmem is accounted in memory for v2 and a separate tcp
counter for v1. I am fine with either approaches mentioned by Arjun
but I would prefer to not add complexity by doing one approach for v1
and the other for v2.