On Wed, Apr 9, 2025 at 6:09 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 4/10/2025 7:40 AM, Andrii Nakryiko wrote:
> > On Mon, Apr 7, 2025 at 7:24 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
> >> Hi,
> >>
> >> On 4/8/2025 12:05 AM, Andrii Nakryiko wrote:
> >>> On Sun, Apr 6, 2025 at 7:47 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
> >>>> Hi,
> >>>>
> >>>> On 4/5/2025 1:58 AM, Andrii Nakryiko wrote:
> >>>>> On Thu, Mar 27, 2025 at 1:23 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
> >>>>>> From: Hou Tao <houtao1@xxxxxxxxxx>
> >>>>>>
> >>>>>> Add three positive test cases to test the basic operations on the
> >>>>>> dynptr-keyed hash map. The basic operations include lookup, update,
> >>>>>> delete and get_next_key. These operations are exercised both through
> >>>>>> bpf syscall and bpf program. These three test cases use different map
> >>>>>> keys. The first test case uses both bpf_dynptr and a struct with only
> >>>>>> bpf_dynptr as map key, the second one uses a struct with an integer and
> >>>>>> a bpf_dynptr as map key, and the last one use a struct with two
> >>>>>> bpf_dynptr as map key: one in the struct itself and another is nested in
> >>>>>> another struct.
> >>>>>>
> >>>>>> Also add multiple negative test cases for dynptr-keyed hash map. These
> >>>>>> test cases mainly check whether the layout of dynptr and non-dynptr in
> >>>>>> the stack is matched with the definition of map->key_record.
> >>>>>>
> >>>>>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
> >>>>>> ---
> >>>>>> .../bpf/prog_tests/htab_dynkey_test.c | 446 ++++++++++++++++++
> >>>>>> .../bpf/progs/htab_dynkey_test_failure.c | 266 +++++++++++
> >>>>>> .../bpf/progs/htab_dynkey_test_success.c | 382 +++++++++++++++
> >>>>>> 3 files changed, 1094 insertions(+)
> >>>>>> create mode 100644 tools/testing/selftests/bpf/prog_tests/htab_dynkey_test.c
> >>>>>> create mode 100644 tools/testing/selftests/bpf/progs/htab_dynkey_test_failure.c
> >>>>>> create mode 100644 tools/testing/selftests/bpf/progs/htab_dynkey_test_success.c
> >>>>>>
[...]
> >>>>>> + /* Lookup it again */
> >>>>>> + value = bpf_map_lookup_elem(htab, &key);
> >>>>>> + bpf_ringbuf_discard_dynptr(&key.f_3.name, 0);
> >>>>>> + if (value) {
> >>>>>> + err = 12;
> >>>>>> + goto out;
> >>>>>> + }
> >>>>>> +out:
> >>>>>> + return err;
> >>>>>> +}
> >>>>> So, I'm not a big fan of this approach of literally embedding struct
> >>>>> bpf_dynptr into map key type and actually initializing and working
> >>>>> with it directly, like you do here with
> >>>>> bpf_ringbuf_reserve_dynptr(..., &key.f_3.name).
> >>>>>
> >>>>> Here's why. This approach only works for *map keys* (not map values)
> >>>>> and only when **the copy of the key** is on the stack (i.e., for map
> >>>>> lookups/updates/deletes). This approach won't work for having dynptrs
> >>>>> inside map value (for variable sized map values), nor does it really
> >>>>> work when you get a direct pointer to map key in
> >>>>> bpf_for_each_map_elem().
> >>>> Yes. The reason why the key should be on the stack is due to the
> >>>> limitation (or the design) of bpf_dynptr. However I didn't understand
> >>>> why it doesn't work for map value just like other special field in the
> >>>> map value (e.g., bpf_timer) ?
> >>> bpf_timer and other special things that go into map_value have to
> >>> painfully and carefully handle simultaneous access and modification of
> >>> map value. So they either do locking (and thus are not compatible or
> >>> reliable under NMI), or would need to be implemented locklessly.
> >>>
> >>> Dynptr is by design assumed to not be dealing with concurrent
> >>> modifications, so bpf_dynptr_adjust(), for instance, can just update
> >>> offset in place without any locking. Reliably and quickly.
> >> Thanks for the explanation here and below. I got it now: multiple bpf
> >> program could get the same map value through lookup and modify it
> >> concurrently through helpers or kfuncs. A bit of slow for me to figure
> >> out by myself. However, I think there is a big difference between
> >> bpf_dynptr and bpf_timer or other special fields. For bpf_timer, we
> >> could not update it through bpf_map_update_elem, so extra helpers or
> >> kfuncs are needed. However, for bpf_dynptr in map key/value, it could be
> >> updated through bpf_map_{update|delete}_elem(). Therefore, for dynptr in
> >> map key or map value, does it really need to allow update through
> >> non-map-update helpers and kfuncs ? Will it be enough to make the dynptr
> >> in map key/value be read-only ? If the dynptr in map key could be
> >> modified by bpf_dyptr_adjust(), the lookup procedure may fail to find
> >> the target map element.
> > So you are saying that we would allow to update dynptr using
> > bpf_map_update_elem() (from BPF side), but allow to use bpf_dynptr
> > read-only APIs directly on dynptr inside the map_value, is that right?
>
> Yes. In your original stashed dynptr proposal, updating the dynptr part
> in the map key is done through bpf_map_update_elem(). For map value, do
> you think it is OK that we disable the update of dynptr in map value
> through bpf_map_update_elem() and only allow the update through extra
> kfuncs just like the existing special field in map value ? It will make
> a difference between the update of map key and map value. Will try to
> check whether is feasible to support the update of dynptr in map value
> through both bpf_map_update_elem() and extra kfunc.
I don't really see a problem supporting both. Under the cover it will
be the same logic, shared internal helper function, probably.
But if for whatever reason that's impossible, we can restrict setting
a new bpf_dynptr through bpf_map_update_elem(), yes.
[...]
> >
> >> 2) need to support concurrent update through non-bpf_map_update_elem helpers
> >> However, if the dynptr in map key and value is read-only, there will be
> >> no concurrent update. The update could be done through
> >> bpf_map_update_elem helper.
> >>
> >> 3) need to support in-place update through bpf_map_update_elem helper
> >> (e.g., for per-CPU map)
> >> However, if we need to support dynptr in map value, maybe we should
> >> change the in-place update to out-of-place update.
> >>
> >> Hope I didn't miss any point.
> > So, basically, if we unnecessarily restrict usability of dynptr, we
> > might carve out some way to work with dynptrs inside map key (but not
> > really map value, as I explained).
> >
> > On the other hand, we can have "stashed dynptr" and make it work the
> > same way in all situations.
> >
> > Tough choice, isn't it? ;)
>
> I see. Thanks for all these explanations. Will switch to the "stashed
> dynptr" in v4.
great, let's give it a try and see if we missed any gotchas, thanks!
> >
> >>>>> struct id_dname_key {
> >>>>> int id;
> >>>>> struct bpf_stashed_dynptr name;
> >>>>> };
> >>>>>
> >>> [...]
> >>>
[...]
