[bpf-next,2/3] bpf: implement map_update_elem to init relay file

Commit Message

Philo Lu Dec. 22, 2023, 12:21 p.m. UTC

map_update_elem is used to create relay files and bind them with the
relay channel, which is created with BPF_MAP_CREATE. This allows users
to set a custom directory name. It must be used with key=NULL and
flag=0.

Here is an example:
```
struct {
__uint(type, BPF_MAP_TYPE_RELAY);
__uint(max_entries, 4096);
} my_relay SEC(".maps");
...
char dir_name[] = "relay_test";
bpf_map_update_elem(map_fd, NULL, dir_name, 0);
```

Then, directory `/sys/kerenl/debug/relay_test` will be created, which
includes files of my_relay0...my_relay[#cpu]. Each represents a per-cpu
buffer with size 8 * 4096 B (there are 8 subbufs by default, each with
size 4096B).

Signed-off-by: Philo Lu <[e-mail]>
---
 kernel/bpf/relaymap.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

Comments

Jiri Olsa Dec. 22, 2023, 2:45 p.m. UTC | #1

On Fri, Dec 22, 2023 at 08:21:45PM +0800, Philo Lu wrote:
> map_update_elem is used to create relay files and bind them with the
> relay channel, which is created with BPF_MAP_CREATE. This allows users
> to set a custom directory name. It must be used with key=NULL and
> flag=0.
> 
> Here is an example:
> ```
> struct {
> __uint(type, BPF_MAP_TYPE_RELAY);
> __uint(max_entries, 4096);
> } my_relay SEC(".maps");
> ...
> char dir_name[] = "relay_test";
> bpf_map_update_elem(map_fd, NULL, dir_name, 0);
> ```
> 
> Then, directory `/sys/kerenl/debug/relay_test` will be created, which
> includes files of my_relay0...my_relay[#cpu]. Each represents a per-cpu
> buffer with size 8 * 4096 B (there are 8 subbufs by default, each with
> size 4096B).
> 
> Signed-off-by: Philo Lu <[e-mail]>
> ---
>  kernel/bpf/relaymap.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/relaymap.c b/kernel/bpf/relaymap.c
> index d0adc7f67758..588c8de0a4bd 100644
> --- a/kernel/bpf/relaymap.c
> +++ b/kernel/bpf/relaymap.c
> @@ -117,7 +117,37 @@ static void *relay_map_lookup_elem(struct bpf_map *map, void *key)
>  static long relay_map_update_elem(struct bpf_map *map, void *key, void *value,
>  				   u64 flags)
>  {
> -	return -EOPNOTSUPP;
> +	struct bpf_relay_map *rmap;
> +	struct dentry *parent;
> +	int err;
> +
> +	if (unlikely(flags))
> +		return -EINVAL;
> +
> +	if (unlikely(key))
> +		return -EINVAL;
> +
> +	rmap = container_of(map, struct bpf_relay_map, map);
> +
> +	/* The directory already exists */
> +	if (rmap->relay_chan->has_base_filename)
> +		return -EEXIST;
> +
> +	/* Setup relay files. Note that the directory name passed as value should
> +	 * not be longer than map->value_size, including the '\0' at the end.
> +	 */
> +	((char *)value)[map->value_size - 1] = '\0';
> +	parent = debugfs_create_dir(value, NULL);
> +	if (IS_ERR_OR_NULL(parent))
> +		return PTR_ERR(parent);
> +
> +	err = relay_late_setup_files(rmap->relay_chan, map->name, parent);
> +	if (err) {
> +		debugfs_remove_recursive(parent);
> +		return err;
> +	}

looks like this patch could be moved to the previous one?

jirka

> +
> +	return 0;
>  }
>  
>  static long relay_map_delete_elem(struct bpf_map *map, void *key)
> -- 
> 2.32.0.3.g01195cf9f
> 
>

Philo Lu Dec. 23, 2023, 2:55 a.m. UTC | #2

On 2023/12/22 22:45, Jiri Olsa wrote:
> On Fri, Dec 22, 2023 at 08:21:45PM +0800, Philo Lu wrote:
>> map_update_elem is used to create relay files and bind them with the
>> relay channel, which is created with BPF_MAP_CREATE. This allows users
>> to set a custom directory name. It must be used with key=NULL and
>> flag=0.
>>
>> Here is an example:
>> ```
>> struct {
>> __uint(type, BPF_MAP_TYPE_RELAY);
>> __uint(max_entries, 4096);
>> } my_relay SEC(".maps");
>> ...
>> char dir_name[] = "relay_test";
>> bpf_map_update_elem(map_fd, NULL, dir_name, 0);
>> ```
>>
>> Then, directory `/sys/kerenl/debug/relay_test` will be created, which
>> includes files of my_relay0...my_relay[#cpu]. Each represents a per-cpu
>> buffer with size 8 * 4096 B (there are 8 subbufs by default, each with
>> size 4096B).
>>
>> Signed-off-by: Philo Lu <[e-mail]>
>> ---
>>   kernel/bpf/relaymap.c | 32 +++++++++++++++++++++++++++++++-
>>   1 file changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/relaymap.c b/kernel/bpf/relaymap.c
>> index d0adc7f67758..588c8de0a4bd 100644
>> --- a/kernel/bpf/relaymap.c
>> +++ b/kernel/bpf/relaymap.c
>> @@ -117,7 +117,37 @@ static void *relay_map_lookup_elem(struct bpf_map *map, void *key)
>>   static long relay_map_update_elem(struct bpf_map *map, void *key, void *value,
>>   				   u64 flags)
>>   {
>> -	return -EOPNOTSUPP;
>> +	struct bpf_relay_map *rmap;
>> +	struct dentry *parent;
>> +	int err;
>> +
>> +	if (unlikely(flags))
>> +		return -EINVAL;
>> +
>> +	if (unlikely(key))
>> +		return -EINVAL;
>> +
>> +	rmap = container_of(map, struct bpf_relay_map, map);
>> +
>> +	/* The directory already exists */
>> +	if (rmap->relay_chan->has_base_filename)
>> +		return -EEXIST;
>> +
>> +	/* Setup relay files. Note that the directory name passed as value should
>> +	 * not be longer than map->value_size, including the '\0' at the end.
>> +	 */
>> +	((char *)value)[map->value_size - 1] = '\0';
>> +	parent = debugfs_create_dir(value, NULL);
>> +	if (IS_ERR_OR_NULL(parent))
>> +		return PTR_ERR(parent);
>> +
>> +	err = relay_late_setup_files(rmap->relay_chan, map->name, parent);
>> +	if (err) {
>> +		debugfs_remove_recursive(parent);
>> +		return err;
>> +	}
> 
> looks like this patch could be moved to the previous one?
> 

OK, will do it.

> jirka
> 
>> +
>> +	return 0;
>>   }
>>   
>>   static long relay_map_delete_elem(struct bpf_map *map, void *key)
>> -- 
>> 2.32.0.3.g01195cf9f
>>
>>

Hou Tao Dec. 23, 2023, 11:28 a.m. UTC | #3

Hi,

On 12/22/2023 8:21 PM, Philo Lu wrote:
> map_update_elem is used to create relay files and bind them with the
> relay channel, which is created with BPF_MAP_CREATE. This allows users
> to set a custom directory name. It must be used with key=NULL and
> flag=0.
>
> Here is an example:
> ```
> struct {
> __uint(type, BPF_MAP_TYPE_RELAY);
> __uint(max_entries, 4096);
> } my_relay SEC(".maps");
> ...
> char dir_name[] = "relay_test";
> bpf_map_update_elem(map_fd, NULL, dir_name, 0);
> ```
>
> Then, directory `/sys/kerenl/debug/relay_test` will be created, which
> includes files of my_relay0...my_relay[#cpu]. Each represents a per-cpu
> buffer with size 8 * 4096 B (there are 8 subbufs by default, each with
> size 4096B).

It is a little weird. Because the name of the relay file is
$debug_fs_root/$value_name/${map_name}xxx. Could we update it to
$debug_fs_root/$map_name/$value_name/xxx instead ?
> Signed-off-by: Philo Lu <[e-mail]>
> ---
>  kernel/bpf/relaymap.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/relaymap.c b/kernel/bpf/relaymap.c
> index d0adc7f67758..588c8de0a4bd 100644
> --- a/kernel/bpf/relaymap.c
> +++ b/kernel/bpf/relaymap.c
> @@ -117,7 +117,37 @@ static void *relay_map_lookup_elem(struct bpf_map *map, void *key)
>  static long relay_map_update_elem(struct bpf_map *map, void *key, void *value,
>  				   u64 flags)
>  {
> -	return -EOPNOTSUPP;
> +	struct bpf_relay_map *rmap;
> +	struct dentry *parent;
> +	int err;
> +
> +	if (unlikely(flags))
> +		return -EINVAL;
> +
> +	if (unlikely(key))
> +		return -EINVAL;
> +
> +	rmap = container_of(map, struct bpf_relay_map, map);
> +

Lock is needed here, because .map_update_elem can be invoked concurrently.
> +	/* The directory already exists */
> +	if (rmap->relay_chan->has_base_filename)
> +		return -EEXIST;
> +
> +	/* Setup relay files. Note that the directory name passed as value should
> +	 * not be longer than map->value_size, including the '\0' at the end.
> +	 */
> +	((char *)value)[map->value_size - 1] = '\0';
> +	parent = debugfs_create_dir(value, NULL);
> +	if (IS_ERR_OR_NULL(parent))
> +		return PTR_ERR(parent);
> +
> +	err = relay_late_setup_files(rmap->relay_chan, map->name, parent);
> +	if (err) {
> +		debugfs_remove_recursive(parent);
> +		return err;
> +	}
> +
> +	return 0;
>  }
>  
>  static long relay_map_delete_elem(struct bpf_map *map, void *key)

Philo Lu Dec. 23, 2023, 1:19 p.m. UTC | #4

On 2023/12/23 19:28, Hou Tao wrote:
> Hi,
> 
> On 12/22/2023 8:21 PM, Philo Lu wrote:
>> map_update_elem is used to create relay files and bind them with the
>> relay channel, which is created with BPF_MAP_CREATE. This allows users
>> to set a custom directory name. It must be used with key=NULL and
>> flag=0.
>>
>> Here is an example:
>> ```
>> struct {
>> __uint(type, BPF_MAP_TYPE_RELAY);
>> __uint(max_entries, 4096);
>> } my_relay SEC(".maps");
>> ...
>> char dir_name[] = "relay_test";
>> bpf_map_update_elem(map_fd, NULL, dir_name, 0);
>> ```
>>
>> Then, directory `/sys/kerenl/debug/relay_test` will be created, which
>> includes files of my_relay0...my_relay[#cpu]. Each represents a per-cpu
>> buffer with size 8 * 4096 B (there are 8 subbufs by default, each with
>> size 4096B).
> 
> It is a little weird. Because the name of the relay file is
> $debug_fs_root/$value_name/${map_name}xxx. Could we update it to
> $debug_fs_root/$map_name/$value_name/xxx instead ?

I think a unique directory is enough for a relay map, so currently users 
can use map_update_elem to set the directory name. Thus 
$map_name/${value_name}xxx may be better than $map_name/$value_name/xxx.

As for whether map_name or value_name is better to be used as the 
directory name, I think it more likely that different bpf programs share 
a same map_name. So value_name is currently used.

>> Signed-off-by: Philo Lu <[e-mail]>
>> ---
>>   kernel/bpf/relaymap.c | 32 +++++++++++++++++++++++++++++++-
>>   1 file changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/relaymap.c b/kernel/bpf/relaymap.c
>> index d0adc7f67758..588c8de0a4bd 100644
>> --- a/kernel/bpf/relaymap.c
>> +++ b/kernel/bpf/relaymap.c
>> @@ -117,7 +117,37 @@ static void *relay_map_lookup_elem(struct bpf_map *map, void *key)
>>   static long relay_map_update_elem(struct bpf_map *map, void *key, void *value,
>>   				   u64 flags)
>>   {
>> -	return -EOPNOTSUPP;
>> +	struct bpf_relay_map *rmap;
>> +	struct dentry *parent;
>> +	int err;
>> +
>> +	if (unlikely(flags))
>> +		return -EINVAL;
>> +
>> +	if (unlikely(key))
>> +		return -EINVAL;
>> +
>> +	rmap = container_of(map, struct bpf_relay_map, map);
>> +
> 
> Lock is needed here, because .map_update_elem can be invoked concurrently.

Got it. I will fix it in the next version.

Thanks.

>> +	/* The directory already exists */
>> +	if (rmap->relay_chan->has_base_filename)
>> +		return -EEXIST;
>> +
>> +	/* Setup relay files. Note that the directory name passed as value should
>> +	 * not be longer than map->value_size, including the '\0' at the end.
>> +	 */
>> +	((char *)value)[map->value_size - 1] = '\0';
>> +	parent = debugfs_create_dir(value, NULL);
>> +	if (IS_ERR_OR_NULL(parent))
>> +		return PTR_ERR(parent);
>> +
>> +	err = relay_late_setup_files(rmap->relay_chan, map->name, parent);
>> +	if (err) {
>> +		debugfs_remove_recursive(parent);
>> +		return err;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   static long relay_map_delete_elem(struct bpf_map *map, void *key)

Patch

diff --git a/kernel/bpf/relaymap.c b/kernel/bpf/relaymap.c
index d0adc7f67758..588c8de0a4bd 100644
--- a/kernel/bpf/relaymap.c
+++ b/kernel/bpf/relaymap.c
@@ -117,7 +117,37 @@  static void *relay_map_lookup_elem(struct bpf_map *map, void *key)
 static long relay_map_update_elem(struct bpf_map *map, void *key, void *value,
 				   u64 flags)
 {
-	return -EOPNOTSUPP;
+	struct bpf_relay_map *rmap;
+	struct dentry *parent;
+	int err;
+
+	if (unlikely(flags))
+		return -EINVAL;
+
+	if (unlikely(key))
+		return -EINVAL;
+
+	rmap = container_of(map, struct bpf_relay_map, map);
+
+	/* The directory already exists */
+	if (rmap->relay_chan->has_base_filename)
+		return -EEXIST;
+
+	/* Setup relay files. Note that the directory name passed as value should
+	 * not be longer than map->value_size, including the '\0' at the end.
+	 */
+	((char *)value)[map->value_size - 1] = '\0';
+	parent = debugfs_create_dir(value, NULL);
+	if (IS_ERR_OR_NULL(parent))
+		return PTR_ERR(parent);
+
+	err = relay_late_setup_files(rmap->relay_chan, map->name, parent);
+	if (err) {
+		debugfs_remove_recursive(parent);
+		return err;
+	}
+
+	return 0;
 }
 
 static long relay_map_delete_elem(struct bpf_map *map, void *key)