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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 19272C761AF for ; Thu, 23 Mar 2023 17:27:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232071AbjCWR1j (ORCPT ); Thu, 23 Mar 2023 13:27:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38460 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232065AbjCWR1i (ORCPT ); Thu, 23 Mar 2023 13:27:38 -0400 Received: from mail-qv1-xf2a.google.com (mail-qv1-xf2a.google.com [IPv6:2607:f8b0:4864:20::f2a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 525711040F for ; Thu, 23 Mar 2023 10:27:34 -0700 (PDT) Received: by mail-qv1-xf2a.google.com with SMTP id 31so6558455qvc.1 for ; Thu, 23 Mar 2023 10:27:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20210112.gappssmtp.com; s=20210112; t=1679592453; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=z/h2YRuqxkbq2WVy9rSukgPkz7MffMXHVCaEQ8JsfSI=; b=0b1sytAcRxnizGxvw+p+lkYnbDhWMhQxrXGPJPm5sHacyMrIEdNLQJWaWguMuI6iRT gIob4qZ61pz7VHAvob8ln+eERCrG8SD695iKZx3xXdVhxJf2AK68WpjLNHTbfR1+mL/h W7T8K7/p+oIhtsGd2HUyWvFwIy6iY8DRoXXxq5zUNE26HtDLdvCeI65VKxv5w/6Cz/WF fEPo60DJZK/R6TZ+NZeMzWnHn7sJXOyNF2qPFslVrcEcKc1n5cSCHU1BEimYAOU8AJaR A2BbTJLzfny7Nv4w5tfJ4xLxwqSSQOmSgthksXW6BG1tzizPpqODzSDd5+8QtbLdZvtY p2Tw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679592453; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=z/h2YRuqxkbq2WVy9rSukgPkz7MffMXHVCaEQ8JsfSI=; b=nWM2+Fy4HS82GfNkwO5hfnOL6e8T7xIc/nG0x34M+AuEFBLheamIunTJd1ArSGBR5D iBSAGtGIUYA8oNFMZ4yY21O1RIB22Qj07Hm0czQklXgcsG+2vwTbhxL8ixBAO+/gSSD2 AxvlRcxU1FRfp0E/tHeuPjPM7XjvG2d1r093iNSjv456eva/Qly6Nq+sONaI4NPlmoTj b0+hUXyLgd0f4hpz5c/QndlGFEDfEBJ5/3CgkwjxD0qPJkIgXSQ0enqDG5mge5t4zjtm YUJLmHdxBFoQM/GtcpbkF7h/IN0YIX67sxKYjulmB8wvrOjLFlrpEzlD6kKXNHn3OizP 0tug== X-Gm-Message-State: AO0yUKU58whLFPVDGqQ4cFRqcZ4IqmdTlaXWlkLtndi/Yxv2S7w569Xd kAjSzruQuEu4qF3FD/0l9jS3/w== X-Google-Smtp-Source: AK7set8dWjwTTSIaOdmksX5IwKh8t2lW/W7AGyyEyTnTU89teSPpKydWtKCANGNduWs4VknW+c/exA== X-Received: by 2002:a05:6214:5010:b0:56b:fb30:49c6 with SMTP id jo16-20020a056214501000b0056bfb3049c6mr12890918qvb.50.1679592453212; Thu, 23 Mar 2023 10:27:33 -0700 (PDT) Received: from localhost ([2620:10d:c091:400::5:62db]) by smtp.gmail.com with ESMTPSA id 123-20020a370581000000b007456df35859sm9126833qkf.74.2023.03.23.10.27.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Mar 2023 10:27:32 -0700 (PDT) Date: Thu, 23 Mar 2023 13:27:32 -0400 From: Johannes Weiner To: Yosry Ahmed Cc: Tejun Heo , Josef Bacik , Jens Axboe , Zefan Li , Michal Hocko , Roman Gushchin , Shakeel Butt , Muchun Song , Andrew Morton , Vasily Averin , cgroups@vger.kernel.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, bpf@vger.kernel.org Subject: Re: [RFC PATCH 4/7] memcg: sleep during flushing stats in safe contexts Message-ID: <20230323172732.GE739026@cmpxchg.org> References: <20230323040037.2389095-1-yosryahmed@google.com> <20230323040037.2389095-5-yosryahmed@google.com> <20230323155613.GC739026@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Thu, Mar 23, 2023 at 09:01:12AM -0700, Yosry Ahmed wrote: > On Thu, Mar 23, 2023 at 8:56 AM Johannes Weiner wrote: > > > > On Thu, Mar 23, 2023 at 04:00:34AM +0000, Yosry Ahmed wrote: > > > @@ -644,26 +644,26 @@ static void __mem_cgroup_flush_stats(void) > > > return; > > > > > > flush_next_time = jiffies_64 + 2*FLUSH_TIME; > > > - cgroup_rstat_flush(root_mem_cgroup->css.cgroup, false); > > > + cgroup_rstat_flush(root_mem_cgroup->css.cgroup, may_sleep); > > > > How is it safe to call this with may_sleep=true when it's holding the > > stats_flush_lock? > > stats_flush_lock is always called with trylock, it is only used today > so that we can skip flushing if another cpu is already doing a flush > (which is not 100% correct as they may have not finished flushing yet, > but that's orthogonal here). So I think it should be safe to sleep as > no one can be blocked waiting for this spinlock. I see. It still cannot sleep while the lock is held, though, because preemption is disabled. Make sure you have all lock debugging on while testing this. > Perhaps it would be better semantically to replace the spinlock with > an atomic test and set, instead of having a lock that can only be used > with trylock? It could be helpful to clarify what stats_flush_lock is protecting first. Keep in mind that locks should protect data, not code paths. Right now it's doing multiple things: 1. It protects updates to stats_flush_threshold 2. It protects updates to flush_next_time 3. It serializes calls to cgroup_rstat_flush() based on those ratelimits However, 1. stats_flush_threshold is already an atomic 2. flush_next_time is not atomic. The writer is locked, but the reader is lockless. If the reader races with a flush, you could see this: if (time_after(jiffies, flush_next_time)) spin_trylock() flush_next_time = now + delay flush() spin_unlock() spin_trylock() flush_next_time = now + delay flush() spin_unlock() which means we already can get flushes at a higher frequency than FLUSH_TIME during races. But it isn't really a problem. The reader could also see garbled partial updates, so it needs at least READ_ONCE and WRITE_ONCE protection. 3. Serializing cgroup_rstat_flush() calls against the ratelimit factors is currently broken because of the race in 2. But the race is actually harmless, all we might get is the occasional earlier flush. If there is no delta, the flush won't do much. And if there is, the flush is justified. In summary, it seems to me the lock can be ditched altogether. All the code needs is READ_ONCE/WRITE_ONCE around flush_next_time.