Re: [PATCH RFC bpf-next v1 05/32] bpf: Support kptrs in local storage maps

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

On 9/4/22 1:41 PM, Kumar Kartikeya Dwivedi wrote:
> diff --git a/include/linux/bpf_local_storage.h b/include/linux/bpf_local_storage.h
> index 7ea18d4da84b..6786d00f004e 100644
> --- a/include/linux/bpf_local_storage.h
> +++ b/include/linux/bpf_local_storage.h
> @@ -74,7 +74,7 @@ struct bpf_local_storage_elem {
>   	struct hlist_node snode;	/* Linked to bpf_local_storage */
>   	struct bpf_local_storage __rcu *local_storage;
>   	struct rcu_head rcu;
> -	/* 8 bytes hole */
> +	struct bpf_map *map;		/* Only set for bpf_selem_free_rcu */

Instead of adding another map ptr and using the last 8 bytes hole,

>   	/* The data is stored in another cacheline to minimize
>   	 * the number of cachelines access during a cache hit.
>   	 */
> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> index 802fc15b0d73..4a725379d761 100644
> --- a/kernel/bpf/bpf_local_storage.c
> +++ b/kernel/bpf/bpf_local_storage.c
> @@ -74,7 +74,8 @@ bpf_selem_alloc(struct bpf_local_storage_map *smap, void *owner,
>   				gfp_flags | __GFP_NOWARN);
>   	if (selem) {
>   		if (value)
> -			memcpy(SDATA(selem)->data, value, smap->map.value_size);
> +			copy_map_value(&smap->map, SDATA(selem)->data, value);
> +		/* No call to check_and_init_map_value as memory is zero init */
>   		return selem;
>   	}
>   
> @@ -92,12 +93,27 @@ void bpf_local_storage_free_rcu(struct rcu_head *rcu)
>   	kfree_rcu(local_storage, rcu);
>   }
>   
> +static void check_and_free_fields(struct bpf_local_storage_elem *selem)
> +{
> +	if (map_value_has_kptrs(selem->map))

could SDATA(selem)->smap->map be used here ?

> +		bpf_map_free_kptrs(selem->map, SDATA(selem));
> +}
> +
>   static void bpf_selem_free_rcu(struct rcu_head *rcu)
>   {
>   	struct bpf_local_storage_elem *selem;
>   
>   	selem = container_of(rcu, struct bpf_local_storage_elem, rcu);
> -	kfree_rcu(selem, rcu);
> +	check_and_free_fields(selem);
> +	kfree(selem);
> +}
> +
> +static void bpf_selem_free_tasks_trace_rcu(struct rcu_head *rcu)
> +{
> +	struct bpf_local_storage_elem *selem;
> +
> +	selem = container_of(rcu, struct bpf_local_storage_elem, rcu);
> +	call_rcu(&selem->rcu, bpf_selem_free_rcu);
>   }
>   
>   /* local_storage->lock must be held and selem->local_storage == local_storage.
> @@ -150,10 +166,11 @@ bool bpf_selem_unlink_storage_nolock(struct bpf_local_storage *local_storage,
>   	    SDATA(selem))
>   		RCU_INIT_POINTER(local_storage->cache[smap->cache_idx], NULL);
>   
> +	selem->map = &smap->map;
>   	if (use_trace_rcu)
> -		call_rcu_tasks_trace(&selem->rcu, bpf_selem_free_rcu);
> +		call_rcu_tasks_trace(&selem->rcu, bpf_selem_free_tasks_trace_rcu);
>   	else
> -		kfree_rcu(selem, rcu);
> +		call_rcu(&selem->rcu, bpf_selem_free_rcu);
>