[PATCH bpf-next v2 1/4] bpf: Generalize bpf_sk_storage

[Martin KaFai Lau]

On Tue, Jun 30, 2020 at 10:00:18PM +0000, KP Singh wrote:
> 
> 
> On Tue, Jun 30, 2020 at 9:35 PM Martin KaFai Lau <kafai at fb.com> wrote:
> >
> > On Mon, Jun 29, 2020 at 06:01:00PM +0200, KP Singh wrote:
> > > > >
> 
> [...]
> 
> > > > >  static atomic_t cache_idx;
> > > > inode local storage and sk local storage probably need a separate
> > > > cache_idx.  An improvement on picking cache_idx has just been
> > > > landed also.
> > >
> > > I see, thanks! I rebased and I now see that cache_idx is now a:
> > >
> > >   static u64 cache_idx_usage_counts[BPF_STORAGE_CACHE_SIZE];
> > >
> > > which tracks the free cache slots rather than using a single atomic
> > > cache_idx. I guess all types of local storage can share this now
> > > right?
> > I believe they have to be separated.  A sk-storage will not be cached/stored
> > in inode.  Caching a sk-storage at idx=0 of a sk should not stop
> > an inode-storage to be cached at the same idx of a inode.
> 
> Ah yes, I see.
> 
> I came up with some macros to solve this. Let me know what you think:
> (this is on top of the refactoring I did, so some function names may seem new,
> but it should, hopefully, convey the general idea).
> 
> diff --git a/include/linux/bpf_local_storage.h b/include/linux/bpf_local_storage.h
> index 3067774cc640..1dc2e6d72091 100644
> --- a/include/linux/bpf_local_storage.h
> +++ b/include/linux/bpf_local_storage.h
> @@ -79,6 +79,26 @@ struct bpf_local_storage_elem {
>  #define SDATA(_SELEM) (&(_SELEM)->sdata)
>  #define BPF_STORAGE_CACHE_SIZE	16
>  
> +u16 bpf_ls_cache_idx_get(spinlock_t *cache_idx_lock,
> +			   u64 *cache_idx_usage_count);
> +
> +void bpf_ls_cache_idx_free(spinlock_t *cache_idx_lock,
> +			   u64 *cache_idx_usage_counts, u16 idx);
> +
> +#define DEFINE_BPF_STORAGE_CACHE(type)					\
> +static DEFINE_SPINLOCK(cache_idx_lock_##type);				\
> +static u64 cache_idx_usage_counts_##type[BPF_STORAGE_CACHE_SIZE];	\
> +static u16 cache_idx_get_##type(void)					\
> +{									\
> +	return bpf_ls_cache_idx_get(&cache_idx_lock_##type,		\
> +				    cache_idx_usage_counts_##type);	\
> +}									\
> +static void cache_idx_free_##type(u16 idx)				\
> +{									\
> +	return bpf_ls_cache_idx_free(&cache_idx_lock_##type,		\
> +				     cache_idx_usage_counts_##type,	\
> +				     idx);				\
> +}
Sorry for the late reply.  I missed this email.

The above looks reasonable.