From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-18.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E58F0C433C1 for ; Tue, 23 Mar 2021 18:42:19 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 5A85A619A3 for ; Tue, 23 Mar 2021 18:42:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5A85A619A3 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B4FDD6B0290; Tue, 23 Mar 2021 14:42:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B00E96B0291; Tue, 23 Mar 2021 14:42:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 97B2D6B0292; Tue, 23 Mar 2021 14:42:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0078.hostedemail.com [216.40.44.78]) by kanga.kvack.org (Postfix) with ESMTP id 785276B0290 for ; Tue, 23 Mar 2021 14:42:18 -0400 (EDT) Received: from smtpin06.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 2C812181AC9CB for ; Tue, 23 Mar 2021 18:42:18 +0000 (UTC) X-FDA: 77952009156.06.54F1CB8 Received: from mail-pj1-f45.google.com (mail-pj1-f45.google.com [209.85.216.45]) by imf20.hostedemail.com (Postfix) with ESMTP id DFDE6FA for ; Tue, 23 Mar 2021 18:42:16 +0000 (UTC) Received: by mail-pj1-f45.google.com with SMTP id ha17so10502887pjb.2 for ; Tue, 23 Mar 2021 11:42:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mD61Ls6vzVvQMSeaaF56QK5c7Jjn0dunNUYdNfL29o0=; b=Yxm1As2H3pEmc1Sg12rZKhMnJyeu07K+JLnvdk3QBaxxYlJEzDmemZrDF0Ip283m2n KvVcd/fnExUfAMVMlmsFS66ObnPWv6O00ZWkyipAsvnTig4pl3MO9WFA5Ors5hmGE2+E ITg2Vq10vzklKs4BrsWhXCdPmS6fEjxhlMCCo2GQantmnqtu+FevhJFfJG9IGcDfEmIc eBa34K/i+gPMCtu40dgX+BI7B/YgufwBp+AZzEepmWuDvlPwyqr8MMtabVl1weobp4T4 1QkNtv0dJIA+oSu1BPQcgGO/WR+0tCg22lXERtZFxhTmTZH92pb3+dpaCtHTRebJ6M1z 2dPA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mD61Ls6vzVvQMSeaaF56QK5c7Jjn0dunNUYdNfL29o0=; b=eZNxcWQ8zSXvvQQ4af8TJ288h2KM8jN6GYHSKmJx6/1qeKuzwjVkqHjMbZNZMEGw0R +IF/+Z5NXxXGVYXXQP80SxUSFSgBKAEfdoA4rgWcxA3UJaAgMztN8sWDBFW11gGVGaN3 T5y2IMDl5/xYgZGIN1rcfbuQdonSopkwdnpLt6XpAVJTDb6PWYoghJ17DM5ntNwCUDm+ bYmr/z2M2+5LZUpDNAYK/dai+ybbtjqHFCLTfgZ2t/v97xnHP5CSCCHEOb1x2QaHX4+o JUF5KdhUhOCT5WRG6BbdO1XlY+m6WNS1pObNYPAxkRMludnxRf0Z819sQFKobwXapd4G wGSg== X-Gm-Message-State: AOAM532OOEgoX7mrtcZUZlCU8TBa+JAze/WgrWNtoa1xx5cRdcq/gwcX Iv1AardtHFPZFBdVoklKlZjJHMIgGuvToOQ4OXDkdw== X-Google-Smtp-Source: ABdhPJygvpHW1+nS6pqN04XtniIKRXltnxLG60Gv6sngXjpNYOucsvIhxX3LDXIdlIDxgmV7t00gIDNVyweZG0ihoMQ= X-Received: by 2002:a17:90a:ce0d:: with SMTP id f13mr5731015pju.85.1616524936259; Tue, 23 Mar 2021 11:42:16 -0700 (PDT) MIME-Version: 1.0 References: <20210316041645.144249-1-arjunroy.kdev@gmail.com> In-Reply-To: From: Arjun Roy Date: Tue, 23 Mar 2021 11:42:05 -0700 Message-ID: Subject: Re: [mm, net-next v2] mm: net: memcg accounting for TCP rx zerocopy To: Johannes Weiner Cc: Arjun Roy , Andrew Morton , David Miller , netdev , Linux Kernel Mailing List , Cgroups , Linux MM , Shakeel Butt , Eric Dumazet , Soheil Hassas Yeganeh , Jakub Kicinski , Michal Hocko , Yang Shi , Roman Gushchin Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: d189jiaexcyom1ycbagjj1a8rpp3b6q4 X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: DFDE6FA Received-SPF: none (google.com>: No applicable sender policy available) receiver=imf20; identity=mailfrom; envelope-from=""; helo=mail-pj1-f45.google.com; client-ip=209.85.216.45 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1616524936-264608 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Mar 23, 2021 at 10:01 AM Johannes Weiner wrote: > > On Mon, Mar 22, 2021 at 02:35:11PM -0700, Arjun Roy wrote: > > To make sure we're on the same page, then, here's a tentative > > mechanism - I'd rather get buy in before spending too much time on > > something that wouldn't pass muster afterwards. > > > > A) An opt-in mechanism, that a driver needs to explicitly support, in > > order to get properly accounted receive zerocopy. > > Yep, opt-in makes sense. That allows piece-by-piece conversion and > avoids us having to have a flag day. > > > B) Failure to opt-in (e.g. unchanged old driver) can either lead to > > unaccounted zerocopy (ie. existing behaviour) or, optionally, > > effectively disabled zerocopy (ie. any call to zerocopy will return > > something like EINVAL) (perhaps controlled via some sysctl, which > > either lets zerocopy through or not with/without accounting). > > I'd suggest letting it fail gracefully (i.e. no -EINVAL) to not > disturb existing/working setups during the transition period. But the > exact policy is easy to change later on if we change our minds on it. > > > The proposed mechanism would involve: > > 1) Some way of marking a page as being allocated by a driver that has > > decided to opt into this mechanism. Say, a page flag, or a memcg flag. > > Right. I would stress it should not be a memcg flag or any direct > channel from the network to memcg, as this would limit its usefulness > while having the same maintenance overhead. > > It should make the network page a first class MM citizen - like an LRU > page or a slab page - which can be accounted and introspected as such, > including from the memcg side. > > So definitely a page flag. Works for me. > > > 2) A callback provided by the driver, that takes a struct page*, and > > returns a boolean. The value of the boolean being true indicates that > > any and all refs on the page are held by the driver. False means there > > exists at least one reference that is not held by the driver. > > I was thinking the PageNetwork flag would cover this, but maybe I'm > missing something? > The main reason for a driver callback is to handle whatever driver-specific behaviour needs to be handled (ie. while a driver may use code from net/core/page_pool.c, it also may roll its own arbitrary behaviour and data structures). And because it's not necessarily the case that a driver would take exactly 1 ref of its own on the page. > > 3) A branch in put_page() that, for pages marked thus, will consult > > the driver callback and if it returns true, will uncharge the memcg > > for the page. > > The way I picture it, put_page() (and release_pages) should do this: > > void __put_page(struct page *page) > { > if (is_zone_device_page(page)) { > put_dev_pagemap(page->pgmap); > > /* > * The page belongs to the device that created pgmap. Do > * not return it to page allocator. > */ > return; > } > + > + if (PageNetwork(page)) { > + put_page_network(page); > + /* Page belongs to the network stack, not the page allocator */ > + return; > + } > > if (unlikely(PageCompound(page))) > __put_compound_page(page); > else > __put_single_page(page); > } > > where put_page_network() is the network-side callback that uncharges > the page. > > (..and later can be extended to do all kinds of things when informed > that the page has been freed: update statistics (mod_page_state), put > it on a private network freelist, or ClearPageNetwork() and give it > back to the page allocator etc. > Yes, this is more or less what I had in mind, though put_page_network() would also need to avail itself of the callback mentioned previously. > But for starters it can set_page_count(page, 1) after the uncharge to > retain the current silent recycling behavior.) > This would be one example of where the driver could conceivably have >1 ref for whatever reason (https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/mellanox/mlx4/en_rx.c#L495) where it looks like it could take 2 refs on a page, perhaps storing 2 x 1500B packets on a single 4KB page. > > The anonymous struct you defined above is part of a union that I think > > normally is one qword in length (well, could be more depending on the > > typedefs I saw there) and I think that can be co-opted to provide the > > driver callback - though, it might require growing the struct by one > > more qword since there may be drivers like mlx5 that are already using > > the field already in there for dma_addr. > > The page cache / anonymous struct it's shared with is 5 words (double > linked list pointers, mapping, index, private), and the network struct > is currently one word, so you can add 4 words to a PageNetwork() page > without increasing the size of struct page. That should be plenty of > space to store auxiliary data for drivers, right? > Ah, I think I was looking more narrowly at an older version of the struct. The new one is much easier to parse. :) 4 words should be plenty, I think. > > Anyways, the callback could then be used by the driver to handle the > > other accounting quirks you mentioned, without needing to scan the > > full pool. > > Right. > > > Of course there are corner cases and such to properly account for, but > > I just wanted to provide a really rough sketch to see if this > > (assuming it were properly implemented) was what you had in mind. If > > so I can put together a v3 patch. > > Yeah, makes perfect sense. We can keep iterating like this any time > you feel you accumulate too many open questions. Not just for MM but > also for the networking folks - although I suspect that the first step > would be mostly about the MM infrastructure, and I'm not sure how much > they care about the internals there ;) > > > Per my response to Andrew earlier, this would make it even more > > confusing whether this is to be applied against net-next or mm trees. > > But that's a bridge to cross when we get to it. > > The mm tree includes -next, so it should be a safe development target > for the time being. > > I would then decide it based on how many changes your patch interacts > with on either side. Changes to struct page and the put path are not > very frequent, so I suspect it'll be easy to rebase to net-next and > route everything through there. And if there are heavy changes on both > sides, the -mm tree is the better route anyway. > > Does that sound reasonable? This sounds good to me. To summarize then, it seems to me that we're on the same page now. I'll put together a tentative v3 such that: 1. It uses pre-charging, as previously discussed. 2. It uses a page flag to delineate pages of a certain networking sort (ie. this mechanism). 3. It avails itself of up to 4 words of data inside struct page, inside the networking specific struct. 4. And it sets up this opt-in lifecycle notification for drivers that choose to use it, falling back to existing behaviour without. Thanks, -Arjun