Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Tue, 5 Apr 2022 13:38:18 -0700
From:   Martin KaFai Lau <kafai@fb.com>
To:     KP Singh <kpsingh@kernel.org>
Cc:     Neeraj Upadhyay <quic_neeraju@quicinc.com>, paulmck@kernel.org,
        Eric Dumazet <edumazet@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Stanislav Fomichev <sdf@google.com>, andrii@kernel.org,
        ast@kernel.org, bpf <bpf@vger.kernel.org>
Subject: Re: [BUG] rcu-tasks : should take care of sparse cpu masks
Message-ID: <20220405203818.qsi7j74jpsex7oky@kafai-mbp.dhcp.thefacebook.com>
References: <CANn89i+rfrkRrdYAq8Baq04n_ACq+VdB+UcsMoq7U-dB-2hKJA@mail.gmail.com>
 <20220401000642.GB4285@paulmck-ThinkPad-P17-Gen-1>
 <CANn89iJtfTiSz4v+L3YW+b_gzNoPLz_wuAmXGrNJXqNs9BU9cA@mail.gmail.com>
 <20220401130114.GC4285@paulmck-ThinkPad-P17-Gen-1>
 <CANn89iLicuKS2wDjY1D5qNT4c-ob=D2n1NnRnm5fGg4LFuW1Kg@mail.gmail.com>
 <20220401152037.GD4285@paulmck-ThinkPad-P17-Gen-1>
 <20220401152814.GA2841044@paulmck-ThinkPad-P17-Gen-1>
 <20220401154837.GA2842076@paulmck-ThinkPad-P17-Gen-1>
 <7a90a9b5-df13-6824-32d1-931f19c96cba@quicinc.com>
 <CACYkzJ4FzbFu5NfdRMParp3Ome=ygVAqQPs2v6UGzRDt2LC6iw@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CACYkzJ4FzbFu5NfdRMParp3Ome=ygVAqQPs2v6UGzRDt2LC6iw@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?QzJZPXpnULOovVS8WOFEa4Y18N5SgJKzkBUcXLcMELLeniEBiPFWDKOB715T?=
 =?us-ascii?Q?aV3YL9drKj3ErFM1qlptQ+g4RDx8EmK7ER7qli1AwS5whYn+UPuvM64NDqdb?=
 =?us-ascii?Q?WVgUhEQJ9L0YF8tDjsjgsr2nb/4OvI/Q0WkswDzksCI4S6krhUbtBvtyab/K?=
 =?us-ascii?Q?QCCLIG9cJS8UDQO5/j+ssh2eLd8R3SgP/SYHmYv0IoczUXUHIqQgZhRTSUi6?=
 =?us-ascii?Q?gD0YB41dhPvD3k6aIr/kXcO7kNC4uPIw2HoGrgLXXFDAs5cQpLP+mrReEfl6?=
 =?us-ascii?Q?/ETo2MB0T+DqqjpHn5PZKSe1MD1rpzgoftUuoAdc/Z1jkiGyRFcER1Q/Q2CS?=
 =?us-ascii?Q?u9gG9txqNGdM5ORJjf8Id7kzTRRsLq/2Wqm/Mtlsi8/kthChuEECim8YxHqH?=
 =?us-ascii?Q?5/Q1Fxx4KPCtxAiibfOgWgqOwpGZcHId1fmGDQKztiTLpXV5sCjEWM7/NzMi?=
 =?us-ascii?Q?dNLkCGu87KLCNzUvkqnIIF6Yp5KxQpHYlIjE21XMLFx4KmOZ1RFOdswQDVZI?=
 =?us-ascii?Q?DXDY5uSRwQYBO6nsPj112PoYhCYXLIk5y6YdWPzeSM7yRhT815iqYrUIcU1O?=
 =?us-ascii?Q?o+Jmlm2l62ywRjesvMFVbhtkqA6g77TtwMNbvmyg6RD7coWFOBTArsf+ilPG?=
 =?us-ascii?Q?z99CTaBkzk+v4bXs/SKm7CEEsg8A9O9GU0x+6cxGqZTzCS3n+ZypkkhTMDVq?=
 =?us-ascii?Q?nBanhWSx/apT9epGSOGxi2kjs+jab6ICFJip5Ft3LXzycbexN1otiSnU69Wl?=
 =?us-ascii?Q?JUC3/CcCalIeVv3Y/Ga14NR+5hI2REHALBOIqLS0KdnTKaFxNTQnAVFAgvb5?=
 =?us-ascii?Q?xMVwxjHpCr67y8cBdft3Yg6rNZxG6NlKs8cuGxbn3YOggv8kYYCc1ffiEazy?=
 =?us-ascii?Q?NDnv/pdcGFKfFUPGkMZ9VJPm3S86v8mfaWZEsWgjr3b51nou7Hz8QgeYy1eP?=
 =?us-ascii?Q?iMSUUpCIkJZqF7r06F0GfLxAK3eXsR9NYDxemu7bEdTNzVSRzSPwHXNwxA3b?=
 =?us-ascii?Q?3axtwCIJgDlhBRFoaGnvZlwDA9oqDqgUwGwxSzksHP+MyUbx8mdHaTu/OqCc?=
 =?us-ascii?Q?1eI6wSJvabD7X723TkcGJ3Isurx+KJDjwPeHPhnOUoAP+JIfzp5gQzNJ8LqG?=
 =?us-ascii?Q?2R3zWU3KHRcVhQjAN+sh/mnu3brHXluOVGcIy7YDt6mx1dG5uFmxHVVdllMU?=
 =?us-ascii?Q?QYoQ+9mupHhY2H3A5xmQ21teuD7PuJDAW/YBjgLBWNuiUUPza6EpZmrnrphz?=
 =?us-ascii?Q?dcta+24MHzVi0IRwIFAyowKAs9gryOL3e+KT3A3tEdNJP4CtFNKaf+X6p1Io?=
 =?us-ascii?Q?1K9T1UdDUqfqag+UoMHCUgKJKf8p+0GvpunJVkZtrdZVs6gCRirS7nlFKLZX?=
 =?us-ascii?Q?YlBy8ZtCglrwu7au9CnDuUWU04zsIm1+Fkf05rDz833TccRZ6nxDq8CrnHWl?=
 =?us-ascii?Q?udffhrPz6HQNta0KWrLQgFRIJ/nneLIQOEjbs1a4EyIjSl94Cm/sHYqFcxXZ?=
 =?us-ascii?Q?8Cf05WG4OhsYWXReL+U/rzdbzHtVXhIDMP/Z5Af7wUc7DSEgLeFAVKbc9EQS?=
 =?us-ascii?Q?QjhFPxDp34f+HMxDxVq1YCDpIoyfgutbgVGFV4vGgL0ua3MK2U/kUQqUgo6Q?=
 =?us-ascii?Q?GhnXRcGSVjGsHrRmhjHWjbLngxQi8k05/sDM3f20araeG3r9tUO8NoSXWqvx?=
 =?us-ascii?Q?bBE9Vb7VSfq3R00oUTCR5at4kpFjsGUDSF2pv8G5d1vqPM+NFreh0ozUCixy?=
 =?us-ascii?Q?cviBGUlAPERbD0Mjn9Bu/8eUntrMu90=3D?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6a2da843-c6f3-42e3-8006-08da174438bb
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5016.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Apr 2022 20:38:21.8058
 (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: 8T62uQYzYU/Wjs5CgVe7S9b4SEn+TK7hXkkq3T2kmkLLwfD+dMC2A6dexDYuA/5j
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR15MB5018
X-Proofpoint-ORIG-GUID: AUG7KrGawhcCdhm4pYL46_m1rAsYMCTo
X-Proofpoint-GUID: AUG7KrGawhcCdhm4pYL46_m1rAsYMCTo
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.850,Hydra:6.0.425,FMLib:17.11.64.514
 definitions=2022-04-05_07,2022-04-05_01,2022-02-23_01
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Apr 05, 2022 at 02:04:34AM +0200, KP Singh wrote:
> > >>> Either way, how frequently is call_rcu_tasks_trace() being invoked in
> > >>> your setup?  If it is being invoked frequently, increasing delays would
> > >>> allow multiple call_rcu_tasks_trace() instances to be served by a single
> > >>> tasklist scan.
> > >>>
> > >>>> Given that, I do not think bpf_sk_storage_free() can/should use
> > >>>> call_rcu_tasks_trace(),
> > >>>> we probably will have to fix this soon (or revert from our kernels)
> > >>>
> > >>> Well, you are in luck!!!  This commit added call_rcu_tasks_trace() to
> > >>> bpf_selem_unlink_storage_nolock(), which is invoked in a loop by
> > >>> bpf_sk_storage_free():
> > >>>
> > >>> 0fe4b381a59e ("bpf: Allow bpf_local_storage to be used by sleepable programs")
> > >>>
> > >>> This commit was authored by KP Singh, who I am adding on CC.  Or I would
> > >>> have, except that you beat me to it.  Good show!!!  ;-)
> 
> Hello :)
> 
> Martin, if this ends up being an issue we might have to go with the
> initial proposed approach
> of marking local storage maps explicitly as sleepable so that not all
> maps are forced to be
> synchronized via trace RCU.
> 
> We can make the verifier reject loading programs that try to use
> non-sleepable local storage
> maps in sleepable programs.
> 
> Do you think this is a feasible approach we can take or do you have
> other suggestions?
bpf_sk_storage_free() does not need to use call_rcu_tasks_trace().
The same should go for the bpf_{task,inode}_storage_free().
The sk at this point is being destroyed.  No bpf prog (sleepable or not)
can have a hold on this sk.  The only storage reader left is from
bpf_local_storage_map_free() which is under rcu_read_lock(),
so a 'kfree_rcu(selem, rcu)' is enough.
A few lines below in bpf_sk_storage_free(), 'kfree_rcu(sk_storage, rcu)'
is currently used instead of call_rcu_tasks_trace() for the same reason.

KP, if the above makes sense, can you make a patch for it?
The bpf_local_storage_map_free() code path also does not need
call_rcu_tasks_trace(), so may as well change it together.
The bpf_*_storage_delete() helper and the map_{delete,update}_elem()
syscall still require the call_rcu_tasks_trace().