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

[Philo Lu]

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 <lu...@linux.alibaba.com>
---
  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)