On Thu, Jul 2, 2020 at 4:24 AM Daniel T. Lee <danieltimlee@xxxxxxxxx> wrote:
>
> On Thu, Jul 2, 2020 at 1:34 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Wed, Jul 1, 2020 at 7:17 PM Daniel T. Lee <danieltimlee@xxxxxxxxx> wrote:
> > >
> > > Previously, in order to set the numa_node attribute at the time of map
> > > creation using "libbpf", it was necessary to call bpf_create_map_node()
> > > directly (bpf_load approach), instead of calling bpf_object_load()
> > > that handles everything on its own, including map creation. And because
> > > of this problem, this sample had problems with refactoring from bpf_load
> > > to libbbpf.
> > >
> > > However, by commit 1bdb6c9a1c43 ("libbpf: Add a bunch of attribute
> > > getters/setters for map definitions"), a helper function which allows
> > > the numa_node attribute to be set in the map prior to calling
> > > bpf_object_load() has been added.
> > >
> > > By using libbpf instead of bpf_load, the inner map definition has
> > > been explicitly declared with BTF-defined format. And for this reason
> > > some logic in fixup_map() was not needed and changed or removed.
> > >
> > > Signed-off-by: Daniel T. Lee <danieltimlee@xxxxxxxxx>
> > > ---
> > > samples/bpf/Makefile | 2 +-
> > > samples/bpf/map_perf_test_kern.c | 180 +++++++++++++++----------------
> > > samples/bpf/map_perf_test_user.c | 130 +++++++++++++++-------
> > > 3 files changed, 181 insertions(+), 131 deletions(-)
> > >
> >
> > [...]
> >
> > > +struct inner_lru {
> > > + __uint(type, BPF_MAP_TYPE_LRU_HASH);
> > > + __type(key, u32);
> > > + __type(value, long);
> > > + __uint(max_entries, MAX_ENTRIES);
> > > + __uint(map_flags, BPF_F_NUMA_NODE); /* from _user.c, set numa_node to 0 */
> > > +} inner_lru_hash_map SEC(".maps");
> >
> > you can declaratively set numa_node here with __uint(numa_node, 0),
> > which is actually a default, but for explicitness it's better
> >
>
> It would make _user.c code cleaner, but as you said,
> I'll keep with this implementation.
I meant to do __uint(numa_node, 0) declaratively, even if it's a bit
redundant (because default value is already zero).
user-space bpf_program__numa_node() is inferior for such a simple
static use case.
>
> > > +
> > > +struct {
> > > + __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
> > > + __uint(max_entries, MAX_NR_CPUS);
> > > + __uint(key_size, sizeof(u32));
> > > + __array(values, struct inner_lru); /* use inner_lru as inner map */
> > > +} array_of_lru_hashs SEC(".maps");
> > > +
> >
> > [...]
> >
> > > -static void fixup_map(struct bpf_map_data *map, int idx)
> > > +static void fixup_map(struct bpf_object *obj)
> > > {
> > > + struct bpf_map *map;
> > > int i;
> > >
> > > - if (!strcmp("inner_lru_hash_map", map->name)) {
> > > - inner_lru_hash_idx = idx;
> > > - inner_lru_hash_size = map->def.max_entries;
> > > - }
> > > + bpf_object__for_each_map(map, obj) {
> > > + const char *name = bpf_map__name(map);
> > >
> > > - if (!strcmp("array_of_lru_hashs", map->name)) {
> >
> > I'm a bit too lazy right now to figure out exact logic here, but just
> > wanted to mention that it is possible to statically set inner map
> > elements for array_of_maps and hash_of_maps. Please check
> > tools/testing/selftests/bpf/progs/test_btf_map_in_map.c and see if you
> > can use this feature to simplify this logic a bit.
> >
>
> Thanks for the feedback! But I'm not sure I'm following properly.
>
> If what you are talking about is specifying the inner_map_idx of
> array_of_lru_hashes, I've changed it by using the __array() directives
> of the BTF-defined MAP.
>
> Since inner_map_idx logic has been replaced with BTF-defined map
> definition, the only thing left at here fixup_map() is just resizing map size
> with bpf_map__resize.
Ok, as I said, a bit too lazy to try to figure out the entire logic of
this sample. My point was to check if static initialization of
ARRAY_OF_MAPS/HASH_OF_MAPS elements is doable. If not, it's fine as is
as well.
>
> Thanks for your time and effort for the review.
> Daniel
>
> > > - if (inner_lru_hash_idx == -1) {
> > > - printf("inner_lru_hash_map must be defined before array_of_lru_hashs\n");
> > > - exit(1);
> > > + /* Only change the max_entries for the enabled test(s) */
> > > + for (i = 0; i < NR_TESTS; i++) {
> > > + if (!strcmp(test_map_names[i], name) &&
> > > + (check_test_flags(i))) {
> > > + bpf_map__resize(map, num_map_entries);
> > > + continue;
> > > + }
> > > }
> > > - map->def.inner_map_idx = inner_lru_hash_idx;
> > > - array_of_lru_hashs_idx = idx;
> > > }
> > >
> >
> > [...]
