On Wed, Jan 13, 2021 at 10:48 AM Roman Gushchin <guro@xxxxxx> wrote: > [snip] > > > 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.
