Re: [PATCH bpf-next v8 1/7] A purely mechanical change to split the renaming from the actual generalization.

From: KP Singh <kpsingh@chromium.org>
To: Martin KaFai Lau <kafai@fb.com>
Cc: linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Paul Turner <pjt@google.com>, Jann Horn <jannh@google.com>,
	Florent Revest <revest@chromium.org>
Subject: Re: [PATCH bpf-next v8 1/7] A purely mechanical change to split the renaming from the actual generalization.
Date: Tue, 18 Aug 2020 16:30:21 +0200	[thread overview]
Message-ID: <51554981-21be-5b42-2827-f6c90b587b95@chromium.org> (raw)
In-Reply-To: <20200817235621.ulkqw6mqd2uu647t@kafai-mbp.dhcp.thefacebook.com>

On 8/18/20 1:56 AM, Martin KaFai Lau wrote:
> On Mon, Aug 03, 2020 at 06:46:49PM +0200, KP Singh wrote:
>> From: KP Singh <kpsingh@google.com>
>>
>> Flags/consts:
>>
>>   SK_STORAGE_CREATE_FLAG_MASK	BPF_LOCAL_STORAGE_CREATE_FLAG_MASK
>>   BPF_SK_STORAGE_CACHE_SIZE	BPF_LOCAL_STORAGE_CACHE_SIZE
>>   MAX_VALUE_SIZE		BPF_LOCAL_STORAGE_MAX_VALUE_SIZE
>>
>> Structs:
>>
>>   bucket			bpf_local_storage_map_bucket
>>   bpf_sk_storage_map		bpf_local_storage_map
>>   bpf_sk_storage_data		bpf_local_storage_data
>>   bpf_sk_storage_elem		bpf_local_storage_elem
>>   bpf_sk_storage		bpf_local_storage
>>
>> The "sk" member in bpf_local_storage is also updated to "owner"
>> in preparation for changing the type to void * in a subsequent patch.
>>
>> Functions:
>>
>>   selem_linked_to_sk			selem_linked_to_storage
>>   selem_alloc				bpf_selem_alloc
>>   __selem_unlink_sk			bpf_selem_unlink_storage
>>   __selem_link_sk			bpf_selem_link_storage
>>   selem_unlink_sk			__bpf_selem_unlink_storage
>>   sk_storage_update			bpf_local_storage_update
>>   __sk_storage_lookup			bpf_local_storage_lookup
>>   bpf_sk_storage_map_free		bpf_local_storage_map_free
>>   bpf_sk_storage_map_alloc		bpf_local_storage_map_alloc
>>   bpf_sk_storage_map_alloc_check	bpf_local_storage_map_alloc_check
>>   bpf_sk_storage_map_check_btf		bpf_local_storage_map_check_btf
>>
> 
> [ ... ]
> 
>> @@ -147,85 +148,86 @@ static struct bpf_sk_storage_elem *selem_alloc(struct bpf_sk_storage_map *smap,
>>   * The caller must ensure selem->smap is still valid to be
>>   * dereferenced for its smap->elem_size and smap->cache_idx.
>>   */
>> -static bool __selem_unlink_sk(struct bpf_sk_storage *sk_storage,
>> -			      struct bpf_sk_storage_elem *selem,
>> -			      bool uncharge_omem)
>> +static bool bpf_selem_unlink_storage(struct bpf_local_storage *local_storage,
>> +				     struct bpf_local_storage_elem *selem,
>> +				     bool uncharge_omem)
> Please add a "_nolock()" suffix, just to be clear that the unlink_map()
> counter part is locked.  It could be a follow up later.

Done.

> 
>>  {
>> -	struct bpf_sk_storage_map *smap;
>> -	bool free_sk_storage;
>> +	struct bpf_local_storage_map *smap;
>> +	bool free_local_storage;

[...]

>> +	if (unlikely(!selem_linked_to_storage(selem)))
>>  		/* selem has already been unlinked from sk */
>>  		return;
>>  
>> -	sk_storage = rcu_dereference(selem->sk_storage);
>> -	raw_spin_lock_bh(&sk_storage->lock);
>> -	if (likely(selem_linked_to_sk(selem)))
>> -		free_sk_storage = __selem_unlink_sk(sk_storage, selem, true);
>> -	raw_spin_unlock_bh(&sk_storage->lock);
>> +	local_storage = rcu_dereference(selem->local_storage);
>> +	raw_spin_lock_bh(&local_storage->lock);
>> +	if (likely(selem_linked_to_storage(selem)))
>> +		free_local_storage =
>> +			bpf_selem_unlink_storage(local_storage, selem, true);
>> +	raw_spin_unlock_bh(&local_storage->lock);
>>  
>> -	if (free_sk_storage)
>> -		kfree_rcu(sk_storage, rcu);
>> +	if (free_local_storage)
>> +		kfree_rcu(local_storage, rcu);
>>  }
>>  
>> -static void __selem_link_sk(struct bpf_sk_storage *sk_storage,
>> -			    struct bpf_sk_storage_elem *selem)
>> +static void bpf_selem_link_storage(struct bpf_local_storage *local_storage,
>> +				   struct bpf_local_storage_elem *selem)
> Same here. bpf_selem_link_storage"_nolock"().

Done.

> 
> Please tag the Subject line with "bpf:".

Done. Changed it to:

bpf: Renames in preparation for bpf_local_storage

A purely mechanical change to split the renaming from the actual
generalization.

[...]

> 
> Acked-by: Martin KaFai Lau <kafai@fb.com>
> 