Date:   Fri, 10 Dec 2021 16:38:38 -0800
From:   Martin KaFai Lau <kafai@fb.com>
To:     Pavel Begunkov <asml.silence@gmail.com>
CC:     <netdev@vger.kernel.org>, <bpf@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Song Liu <songliubraving@fb.com>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [BPF PATCH for-next] cgroup/bpf: fast path for not loaded skb
 BPF filtering
Message-ID: <20211211003838.7u4lcqghcq2gqvho@kafai-mbp.dhcp.thefacebook.com>
References: <d77b08bf757a8ea8dab3a495885c7de6ff6678da.1639102791.git.asml.silence@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <d77b08bf757a8ea8dab3a495885c7de6ff6678da.1639102791.git.asml.silence@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?YzablPr3ykuDuaBU3Fg90tyWnM8i3ErzcaWSmU6JFEA7Zx8lrR7Rs57NMgjc?=
 =?us-ascii?Q?VLe8mPLV0BlxacVb520jQVxet9yktLnPqIgeQVcnvn3aR21cp94C/Yr1Fjva?=
 =?us-ascii?Q?xIJsyZIy6Oc5g+JfG/kPQueuNd4Uwbn1FxKd8Fnm6g0oaIANc6tpprP0u84k?=
 =?us-ascii?Q?+40jWa97yCOjg4Fmr6XbJb+nLKmNTMIfpQBLwMqcNwTA2cC6mUbMvvN6veOF?=
 =?us-ascii?Q?rFXUB71GrQaZQbtflyvO0YpwSi6X09g3sLebs9Yi5a/jQv/JdlRhEQzCBzzP?=
 =?us-ascii?Q?AZ+h7cm7kbpl9PLJirAmHiNfXFEWTFidzKWOdabImz9cRhkuy81rlny25Eq+?=
 =?us-ascii?Q?7u3tgSy6nMHvghnv1auGe75ofEZw3ahpalj15GKUBieg8VqsMwd+gL1mQbnj?=
 =?us-ascii?Q?dlwx8y7wKVmNEHE9lbiKX96WTSAPDgoRIpJ3laCSl5SKqJW16HOyCAkBrxtu?=
 =?us-ascii?Q?uVNzvppW4V0BjeTVYaM0zEnPmWqV4/FwwNOVWSZD+D76gqGutDpCgeWn5G8T?=
 =?us-ascii?Q?9GsTMHJ9rnC+Q+XNde8dVDi3smvB02T1fEChbSW8UCiCcPlcvHAqUtBEDUus?=
 =?us-ascii?Q?bTEYh6PQhmrKVIL1ijs84znWln/B2gWAWEIVvM81FgUKXw+tbzRWc+EImDss?=
 =?us-ascii?Q?n4V9eBJkyot8y0oNy2Ij5Z72d/NQf2LyGhEyXKM/1G/J2PsMFdYz79IhU9CF?=
 =?us-ascii?Q?B29e6sWXrUIq/vVqwNB689v8RZ/VRZN5W0FOGckLcR0sJ7WC+ONAY5qTgFRT?=
 =?us-ascii?Q?/8iwmudpOsh8P+4xh3Rx+4g5Lit+Y6mejuQZ0rULgOhJa2S9XM1jeVWdOXK1?=
 =?us-ascii?Q?MnWWi+RZbkOjUj2qK6IB9KEGRndmcc04Ap61MHOi4CAMWqvhEeI3BU/6OKGD?=
 =?us-ascii?Q?8JS3Q+Ad1k1fxb9d2vjoPdcACk8jAtEVyydFoJ2ARb+lGXfbI0uX37wZ6Umh?=
 =?us-ascii?Q?UsP6daVI5QJy4KbCLSCVk+/RpWIe7tGtE+BJzAVZNo6ZMtJmPJc+79/O2bsy?=
 =?us-ascii?Q?eOGHceMgKMFo++bGmfN+R7MZdyKysYwAKnwBVgT+iuWiWPoaWPQ/b2UL7a1k?=
 =?us-ascii?Q?8uQRvjJdgKB+6YjEfCED4Wt0F/5yYML0bwK5GP3tauI5mZ3zf/fvH0sfz+8N?=
 =?us-ascii?Q?hiesMnqlkaVkV4YzVWd/TSdSAcVsrV/Mx9wqkFyOuMim8fcV0tgpeUjIxtVU?=
 =?us-ascii?Q?mG34+RcxKJwL4kO6nPOYVE7rgdnvY64iDgnK0okuZmP8XSqXNKQjjKyj8bJz?=
 =?us-ascii?Q?/xU5wEGDQOpaiCZlx08onmYtjT95PkERwkJYpXIkfgS4WO1YgcCC5nHz9RaC?=
 =?us-ascii?Q?R3XQllkSEip5mNVr+HQk+Rven2JOaudGk/FkDTOny5F4zMnhCS/6bsX5a5bd?=
 =?us-ascii?Q?Nybstx+cfctCdqiNjlMHjoL+5XUtGpDjdEpHNwBQp3FS47hWEzrKpL4LHH6M?=
 =?us-ascii?Q?+MG4IhG+PYTfgF+SHldJ8RRZtqlzB/vlf8iDhg7kBnQ/NGN8AeJ7Vt2jV0dt?=
 =?us-ascii?Q?yqqh3LS4hg8muDjsrvG2aJJQxXq11bzJaQ5iY3xk3mJ7HNgwC6oZvVDVrtP5?=
 =?us-ascii?Q?K5C+AqreXyyZVGlWbOiBdt9sx1zj4eLsCkzxWJnS?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6462e14e-58cb-4395-b55d-08d9bc3e943e
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5016.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Dec 2021 00:38:41.6173
 (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: ShZzw+7mP4pkQ/vhF+J/ibJwKPKzcSnjUrBOajsr6q0bGxXpI/jlxSPhUbQBjSVs
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR15MB4981
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: -gEwKmlgOJ-zpzMbkDXfw4Qih0LYrqNj
X-Proofpoint-ORIG-GUID: -gEwKmlgOJ-zpzMbkDXfw4Qih0LYrqNj
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.11.62.513
 definitions=2021-12-10_09,2021-12-10_01,2021-12-02_01
X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 priorityscore=1501
 phishscore=0 spamscore=0 malwarescore=0 bulkscore=0 impostorscore=0
 adultscore=0 mlxlogscore=552 suspectscore=0 lowpriorityscore=0 mlxscore=0
 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2110150000 definitions=main-2112110001
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Dec 10, 2021 at 02:23:34AM +0000, Pavel Begunkov wrote:
> cgroup_bpf_enabled_key static key guards from overhead in cases where
> no cgroup bpf program of a specific type is loaded in any cgroup. Turn
> out that's not always good enough, e.g. when there are many cgroups but
> ones that we're interesting in are without bpf. It's seen in server
> environments, but the problem seems to be even wider as apparently
> systemd loads some BPF affecting my laptop.
> 
> Profiles for small packet or zerocopy transmissions over fast network
> show __cgroup_bpf_run_filter_skb() taking 2-3%, 1% of which is from
> migrate_disable/enable(), and similarly on the receiving side. Also
> got +4-5% of t-put for local testing.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  include/linux/bpf-cgroup.h | 24 +++++++++++++++++++++---
>  kernel/bpf/cgroup.c        | 23 +++++++----------------
>  2 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index 11820a430d6c..99b01201d7db 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -141,6 +141,9 @@ struct cgroup_bpf {
>  	struct list_head progs[MAX_CGROUP_BPF_ATTACH_TYPE];
>  	u32 flags[MAX_CGROUP_BPF_ATTACH_TYPE];
>  
> +	/* for each type tracks whether effective prog array is not empty */
> +	unsigned long enabled_mask;
> +
>  	/* list of cgroup shared storages */
>  	struct list_head storages;
>  
> @@ -219,11 +222,25 @@ int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key, void *value);
>  int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
>  				     void *value, u64 flags);
>  
> +static inline bool __cgroup_bpf_type_enabled(struct cgroup_bpf *cgrp_bpf,
> +					     enum cgroup_bpf_attach_type atype)
> +{
> +	return test_bit(atype, &cgrp_bpf->enabled_mask);
> +}
> +
> +#define CGROUP_BPF_TYPE_ENABLED(sk, atype)				       \
> +({									       \
> +	struct cgroup *__cgrp = sock_cgroup_ptr(&(sk)->sk_cgrp_data);	       \
> +									       \
> +	__cgroup_bpf_type_enabled(&__cgrp->bpf, (atype));		       \
> +})
I think it should directly test if the array is empty or not instead of
adding another bit.

Can the existing __cgroup_bpf_prog_array_is_empty(cgrp, ...) test be used instead?