Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 5 Mar 2020 14:45:44 -0800
From:   Roman Gushchin <guro@fb.com>
To:     Shakeel Butt <shakeelb@google.com>
CC:     Eric Dumazet <eric.dumazet@gmail.com>,
        Eric Dumazet <edumazet@google.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        "David S . Miller" <davem@davemloft.net>,
        Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        netdev <netdev@vger.kernel.org>, Linux MM <linux-mm@kvack.org>,
        Cgroups <cgroups@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] net: memcg: late association of sock to memcg
Message-ID: <20200305224544.GA1092120@carbon.dhcp.thefacebook.com>
References: <20200305205525.245058-1-shakeelb@google.com>
 <9505d35b-f9fc-149b-6df5-e65ad95acabb@gmail.com>
 <CALvZod68Raqfa2ZJOfF_OOQdb-hxkOs54G5KK3VQnUdxiZ=KTQ@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALvZod68Raqfa2ZJOfF_OOQdb-hxkOs54G5KK3VQnUdxiZ=KTQ@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from carbon.dhcp.thefacebook.com (2620:10d:c090:500::6:48b4) by MWHPR01CA0027.prod.exchangelabs.com (2603:10b6:300:101::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2793.15 via Frontend Transport; Thu, 5 Mar 2020 22:45:47 +0000
X-Originating-IP: [2620:10d:c090:500::6:48b4]
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 800c2384-4534-4115-ef7a-08d7c156f2a3
X-MS-TrafficTypeDiagnostic: MWHPR15MB1790:
X-Microsoft-Antispam-PRVS: <MWHPR15MB17908BE18B0F1013A1A6E265BEE20@MWHPR15MB1790.namprd15.prod.outlook.com>
X-FB-Source: Internal
X-MS-Oob-TLC-OOBClassifiers: OLM:10000;
X-Forefront-PRVS: 03333C607F
X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(366004)(396003)(39860400002)(376002)(346002)(136003)(199004)(189003)(8936002)(9686003)(55016002)(7416002)(6916009)(1076003)(86362001)(4326008)(2906002)(52116002)(66946007)(81166006)(81156014)(5660300002)(7696005)(6506007)(316002)(53546011)(478600001)(66556008)(186003)(8676002)(33656002)(66476007)(6666004)(16526019)(54906003);DIR:OUT;SFP:1102;SCL:1;SRVR:MWHPR15MB1790;H:MWHPR15MB1661.namprd15.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1;
Received-SPF: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: k15srNGbHOJj3rjwNPD+tFPbVF7geOq/KrZ4yxA/WMoUirt24rsJCUNwhQ3tpBK9kcDhtDxhfOExH+ikHRujQbSEnNtR5Cvmqu/xgOGwC+QO1ekfxKJEv+siI4Bf3e323UnvPAq/w6m10O2WuAF9TkJl84w3HSeLHaGNGW6BoEqQgLN7QZNOydIX1lPRt46525S291j9EXxWn+6e5rZEr2rJK3tUitdU1ikY1J696QgvU07VspY0l2smBu/kh7ep2DtzxUPSF/vfzfi6wVOw5ktJWsbfXK4GorMo928pBRRmNWbmKj7uuEU1AevcOeKj2a/ElkeMw964QBJfFH3477kZUlZ9v7V+/zxk1fzXLnw+1hJMGohM3Hvpi5QeeoGP8KNs9HnqUFJHLzuE01CiiS39/A+KJpw1W3BlCE2K8bP2yCTTn0RdxEd6VxIjsFpG
X-MS-Exchange-AntiSpam-MessageData: /0FtUZouI7L/nWCc3fWAa0HA5wp9RgP7EcB6ixjugoDFMT+yUKHxpZpyOQF4EoH7sNtZ3qM3Hzf51+INdPnWGqi/fjrY6wwzON0aJqI3Eu5oBZ/nieN3FOKQ3ObyJsekhq+uta6AMtFXVuE4RWNagKj92fqGIzmeQLPLMRbECr7Q7f49t5MbUe3DkbmtfQgc
X-MS-Exchange-CrossTenant-Network-Message-Id: 800c2384-4534-4115-ef7a-08d7c156f2a3
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Mar 2020 22:45:48.1851
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kuOqZwSFHTIXI2AbY/gGYlsLOD9xOBqscJB+N0nXwAJpgsGbLoO0OQYsCxrrtb6K
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1790
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.572
 definitions=2020-03-05_08:2020-03-05,2020-03-05 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 impostorscore=0
 clxscore=1011 lowpriorityscore=0 phishscore=0 malwarescore=0 adultscore=0
 bulkscore=0 spamscore=0 mlxlogscore=999 mlxscore=0 suspectscore=5
 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2001150001 definitions=main-2003060012
X-FB-Internal: deliver
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Mar 05, 2020 at 01:59:37PM -0800, Shakeel Butt wrote:
> On Thu, Mar 5, 2020 at 1:17 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >
> >
> > On 3/5/20 12:55 PM, Shakeel Butt wrote:
> > > If a TCP socket is allocated in IRQ context or cloned from unassociated
> > > (i.e. not associated to a memcg) in IRQ context then it will remain
> > > unassociated for its whole life. Almost half of the TCPs created on the
> > > system are created in IRQ context, so, memory used by such sockets will
> > > not be accounted by the memcg.
> > >
> > > This issue is more widespread in cgroup v1 where network memory
> > > accounting is opt-in but it can happen in cgroup v2 if the source socket
> > > for the cloning was created in root memcg.
> > >
> > > To fix the issue, just do the late association of the unassociated
> > > sockets at accept() time in the process context and then force charge
> > > the memory buffer already reserved by the socket.
> > >
> > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > > ---
> > > Changes since v2:
> > > - Additional check for charging.
> > > - Release the sock after charging.
> > >
> > > Changes since v1:
> > > - added sk->sk_rmem_alloc to initial charging.
> > > - added synchronization to get memory usage and set sk_memcg race-free.
> > >
> > >  net/ipv4/inet_connection_sock.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > > index a4db79b1b643..5face55cf818 100644
> > > --- a/net/ipv4/inet_connection_sock.c
> > > +++ b/net/ipv4/inet_connection_sock.c
> > > @@ -482,6 +482,26 @@ struct sock *inet_csk_accept(struct sock *sk, int flags, int *err, bool kern)
> > >               }
> > >               spin_unlock_bh(&queue->fastopenq.lock);
> > >       }
> > > +
> > > +     if (mem_cgroup_sockets_enabled && !newsk->sk_memcg) {
> > > +             int amt;
> > > +
> > > +             /* atomically get the memory usage, set and charge the
> > > +              * sk->sk_memcg.
> > > +              */
> > > +             lock_sock(newsk);
> > > +
> > > +             /* The sk has not been accepted yet, no need to look at
> > > +              * sk->sk_wmem_queued.
> > > +              */
> > > +             amt = sk_mem_pages(newsk->sk_forward_alloc +
> > > +                                atomic_read(&sk->sk_rmem_alloc));
> > > +             mem_cgroup_sk_alloc(newsk);
> > > +             if (newsk->sk_memcg && amt)
> > > +                     mem_cgroup_charge_skmem(newsk->sk_memcg, amt);
> > > +
> > > +             release_sock(newsk);
> > > +     }
> > >  out:
> > >       release_sock(sk);
> > >       if (req)
> > >
> >
> > This patch looks fine, but why keeping the mem_cgroup_sk_alloc(newsk);
> > in sk_clone_lock() ?
> >
> > Note that all TCP sk_clone_lock() calls happen in softirq context.
> 
> So, basically re-doing something like 9f1c2674b328 ("net: memcontrol:
> defer call to mem_cgroup_sk_alloc()") in this patch. I am fine with
> that.
> 
> Roman, any concerns?

Nothing at the moment, I'll try to give some testing to the final patch.
I hope this time it will work better...

Thank you!