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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 941D3C4345F for ; Mon, 29 Apr 2024 07:45:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 155706B0087; Mon, 29 Apr 2024 03:45:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 106486B0088; Mon, 29 Apr 2024 03:45:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EE92E6B0089; Mon, 29 Apr 2024 03:45:53 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id D20946B0087 for ; Mon, 29 Apr 2024 03:45:53 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 81F9EA1A34 for ; Mon, 29 Apr 2024 07:45:53 +0000 (UTC) X-FDA: 82061785386.07.B731B66 Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by imf25.hostedemail.com (Postfix) with ESMTP id 0F67DA000D for ; Mon, 29 Apr 2024 07:45:49 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf25.hostedemail.com: domain of zhangpeng362@huawei.com designates 45.249.212.255 as permitted sender) smtp.mailfrom=zhangpeng362@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714376751; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=V6u7nal4YKKCnUZ5l6a6JXFwog0uhtwHNiy8Bdf3UdA=; b=Rv8IoRW/efdrK4fs8Eu6M9X7P/wi/+GCHkurBYrP7oWeeFpssBjt6lLxEy8eSgpx3zoluL Dld/uurwkIsW46ULczuU9tCHgqDaR0VoR5wFR8TrlBrnLJG7BrNlqaFvjFp0vj+0rlpo0e APfQHuI6l0lfLM2szBtlBF7fY/jy0yg= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf25.hostedemail.com: domain of zhangpeng362@huawei.com designates 45.249.212.255 as permitted sender) smtp.mailfrom=zhangpeng362@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714376751; a=rsa-sha256; cv=none; b=AxAmQ/dh82y++I6rDe8OY8PS1LkkA2GFKLm5MWaoUZ7crk+AxiZT1pM3kyxHb/qOBisx4D TgaaWZ+xrXIMa06tI5Qchjv5PGTulXXk3DGdQFep3OWhyDEB+q19isoX2JjDmIzNRvYjnh VvzimLL1kaODb4uuO3XDe1OJCgrcFZk= Received: from mail.maildlp.com (unknown [172.19.163.48]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4VSZzy1DMwz1RBGN; Mon, 29 Apr 2024 15:42:38 +0800 (CST) Received: from kwepemm600020.china.huawei.com (unknown [7.193.23.147]) by mail.maildlp.com (Postfix) with ESMTPS id 10AEF180065; Mon, 29 Apr 2024 15:45:46 +0800 (CST) Received: from [10.174.179.160] (10.174.179.160) by kwepemm600020.china.huawei.com (7.193.23.147) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Mon, 29 Apr 2024 15:45:44 +0800 Message-ID: Date: Mon, 29 Apr 2024 15:45:44 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [RFC PATCH v2 1/2] percpu_counter: introduce atomic mode for percpu_counter Content-Language: en-US To: Dennis Zhou CC: , , , , , , , , , , , , References: <20240418142008.2775308-1-zhangpeng362@huawei.com> <20240418142008.2775308-2-zhangpeng362@huawei.com> From: "zhangpeng (AS)" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.174.179.160] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To kwepemm600020.china.huawei.com (7.193.23.147) X-Stat-Signature: 4uzicybe5iku1ssm5eibmcdj7s4w1591 X-Rspamd-Queue-Id: 0F67DA000D X-Rspamd-Server: rspam10 X-Rspam-User: X-HE-Tag: 1714376749-37323 X-HE-Meta: U2FsdGVkX19BcRJYibcrh6kCE7vp0Gi+N3CU9am3smuJTNrQO7yznMABAvpg40BEwgqUf5fAdeycmt+1ZXieoTXChUlswY1kkdrT/6ZzzLo4dfztEcyjiJTfoEQ/YQ0FpQ6zA4WV9Wq5r3/b3ib4hS0XrH0QhOQNXG0cbwbQfbbjMm/3VpsNmGV9SQP4BGT4D2hVHZsDmZPHBb+T+3p+w5kLN/rSMLzm8yMBwhebh3fK/9h1i+xdOsR8gisCGkIakyaf/y9IVY577hCJmPAqFcnaizqOj0su828WOFjNKKnb9Ys0FFh/ITIyvusSPHCLiA0qo6F5Kdgb7N85h1e/Fk3glnUepsb2jgS5mM4pbbMGGuoOru1UW+rFkDNBrHHZ0dVTrFGmUAkMDSGvXKnKF2UWvBWENuH19uKMWHnQKluglSebr4+xitq4DxntGX+6EyHLH4W4KVBLg1jPj1nyeo4TGYgt9Tpp+SPUz2hoL7lc3lnuPc1mqbGOyIBLb7Suo+fC1G+1g9pdxNb1qyUnc3iEnLfZTpiQxRVVjnCP0hOdrPtA16/fN1ooPCNXc6UMxVeKSlTx+FDpgitRcn1U5mNqaoyTzlpglDilK+gMAg3nsZtk2yHB7hM1z+qMkhRo5AM2rWmpIFKnXQi9+Km2EQa/w2amghSwDYr26emerfZV0v16eLZBadV35Drxor8bA2jYQLUs9so7aLoMF+6vVIUqc2I5CenbpKA5rXgL6RRJmjW12nWube1h1r4pc8HCo0W19REGcCEbowBV34bY1P/YWh3lbNYj3EltqX5ixos4aEhdkOz6M9YJtJ3xXsAnNtyM2IlrAwHpEeiawKZ6UkOwkvEHrvOV+JUSnqrxwGMw/1rt65TOOc08PRUatozWOpArwnQTQRjNXyj0wj3EQ1De25vWH4MJBFEPt9zUjgzse47vSrV2tTm+BaM4gqcEz+bEV/pu54CCmORujrz I1FjDYNQ +SHTRFtG58yD7zxpMlRcLnAvtseIdt+JYje5ffNEthXlEQQMsKxbp3vryKiE3EZ8tSCR2jNNabbh91KCkDy8HMR2pPVvkcM86WLlj+JMXuF0OySOMKF5FJRR6KV/KdZ7AZkHsaiGh71gJkyyPtF1eNZ3XmjoTYTa1FWCAxRhB0JeWErlY+nJ0rE8wh/fkMV9wMJ0i1w96x8KmUYc= 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: List-Subscribe: List-Unsubscribe: On 2024/4/26 16:11, Dennis Zhou wrote: > On Thu, Apr 18, 2024 at 10:20:07PM +0800, Peng Zhang wrote: >> From: ZhangPeng >> >> Depending on whether counters is NULL, we can support two modes: >> atomic mode and perpcu mode. We implement both modes by grouping >> the s64 count and atomic64_t count_atomic in a union. At the same time, >> we create the interface for adding and reading in atomic mode and for >> switching atomic mode to percpu mode. >> >> Suggested-by: Jan Kara >> Signed-off-by: ZhangPeng >> Signed-off-by: Kefeng Wang >> --- >> include/linux/percpu_counter.h | 43 +++++++++++++++++++++++++++++++--- >> lib/percpu_counter.c | 31 ++++++++++++++++++++++-- >> 2 files changed, 69 insertions(+), 5 deletions(-) >> >> diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h >> index 3a44dd1e33d2..160f9734c0bb 100644 >> --- a/include/linux/percpu_counter.h >> +++ b/include/linux/percpu_counter.h >> @@ -21,7 +21,13 @@ >> >> struct percpu_counter { >> raw_spinlock_t lock; >> - s64 count; >> + /* Depending on whether counters is NULL, we can support two modes, >> + * atomic mode using count_atomic and perpcu mode using count. >> + */ >> + union { >> + s64 count; >> + atomic64_t count_atomic; >> + }; >> #ifdef CONFIG_HOTPLUG_CPU >> struct list_head list; /* All percpu_counters are on a list */ >> #endif >> @@ -32,14 +38,14 @@ extern int percpu_counter_batch; >> >> int __percpu_counter_init_many(struct percpu_counter *fbc, s64 amount, >> gfp_t gfp, u32 nr_counters, >> - struct lock_class_key *key); >> + struct lock_class_key *key, bool switch_mode); > Nit: the lock_class_key at the end. Got it. >> >> #define percpu_counter_init_many(fbc, value, gfp, nr_counters) \ >> ({ \ >> static struct lock_class_key __key; \ >> \ >> __percpu_counter_init_many(fbc, value, gfp, nr_counters,\ >> - &__key); \ >> + &__key, false); \ >> }) >> >> >> @@ -130,6 +136,20 @@ static inline bool percpu_counter_initialized(struct percpu_counter *fbc) >> return (fbc->counters != NULL); >> } >> >> +static inline s64 percpu_counter_atomic_read(struct percpu_counter *fbc) >> +{ >> + return atomic64_read(&fbc->count_atomic); >> +} >> + >> +static inline void percpu_counter_atomic_add(struct percpu_counter *fbc, >> + s64 amount) >> +{ >> + atomic64_add(amount, &fbc->count_atomic); >> +} >> + >> +int percpu_counter_switch_to_pcpu_many(struct percpu_counter *fbc, >> + u32 nr_counters); >> + >> #else /* !CONFIG_SMP */ >> >> struct percpu_counter { >> @@ -260,6 +280,23 @@ static inline bool percpu_counter_initialized(struct percpu_counter *fbc) >> static inline void percpu_counter_sync(struct percpu_counter *fbc) >> { >> } >> + >> +static inline s64 percpu_counter_atomic_read(struct percpu_counter *fbc) >> +{ >> + return fbc->count; >> +} >> + >> +static inline void percpu_counter_atomic_add(struct percpu_counter *fbc, >> + s64 amount) >> +{ >> + percpu_counter_add(fbc, amount); >> +} >> + >> +static inline int percpu_counter_switch_to_pcpu_many(struct percpu_counter *fbc, >> + u32 nr_counters) >> +{ >> + return 0; >> +} >> #endif /* CONFIG_SMP */ >> >> static inline void percpu_counter_inc(struct percpu_counter *fbc) >> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c >> index 44dd133594d4..95c4e038051a 100644 >> --- a/lib/percpu_counter.c >> +++ b/lib/percpu_counter.c >> @@ -153,7 +153,7 @@ EXPORT_SYMBOL(__percpu_counter_sum); >> >> int __percpu_counter_init_many(struct percpu_counter *fbc, s64 amount, >> gfp_t gfp, u32 nr_counters, >> - struct lock_class_key *key) >> + struct lock_class_key *key, bool switch_mode) >> { >> unsigned long flags __maybe_unused; >> size_t counter_size; >> @@ -174,7 +174,8 @@ int __percpu_counter_init_many(struct percpu_counter *fbc, s64 amount, >> #ifdef CONFIG_HOTPLUG_CPU >> INIT_LIST_HEAD(&fbc[i].list); >> #endif >> - fbc[i].count = amount; >> + if (likely(!switch_mode)) >> + fbc[i].count = amount; >> fbc[i].counters = (void *)counters + (i * counter_size); >> >> debug_percpu_counter_activate(&fbc[i]); >> @@ -357,6 +358,32 @@ bool __percpu_counter_limited_add(struct percpu_counter *fbc, >> return good; >> } >> >> +/* >> + * percpu_counter_switch_to_pcpu_many: Converts struct percpu_counters from >> + * atomic mode to percpu mode. >> + */ >> +int percpu_counter_switch_to_pcpu_many(struct percpu_counter *fbc, >> + u32 nr_counters) >> +{ >> + static struct lock_class_key __key; > This is an improper use of lockdep. Now all of the percpu_counters > initialized on this path will key off of this lock_class_key. Sorry, I don't know much about lock_class_key. I may not understand the reason why it's not appropriate here. Could you please help me with the details? >> + unsigned long flags; >> + bool ret = 0; >> + >> + if (percpu_counter_initialized(fbc)) >> + return 0; >> + >> + preempt_disable(); >> + local_irq_save(flags); >> + if (likely(!percpu_counter_initialized(fbc))) >> + ret = __percpu_counter_init_many(fbc, 0, >> + GFP_ATOMIC|__GFP_NOWARN|__GFP_ZERO, >> + nr_counters, &__key, true); >> + local_irq_restore(flags); >> + preempt_enable(); >> + >> + return ret; >> +} > I'm staring at this API and I'm not in love with it. I think it hinges > on that there's one user of mm_stats prior hence it's safe. Generically > though, I can't see why this is safe. > > I need to give this a little more thought, but my gut reaction is I'd > rather this look like percpu_refcount where we can init dead minus the > percpu allocation. Maybe that's not quite right, but I'd feel better > about it than requiring external synchronization on a percpu_counter to > ensure that it's correct. Maybe it would be better if I change this API to the internal function of mm counter. Sorry again, maybe percpu_refcount is better, but I don't understand this sentence "we can init dead minus the percpu allocation." Please forgive my ignorance... Thank you very much for your review and valuable comments! >> + >> static int __init percpu_counter_startup(void) >> { >> int ret; >> -- >> 2.25.1 >> > Thanks, > Dennis -- Best Regards, Peng