Eelco Chaudron <echau...@redhat.com> writes:
> On 19 Feb 2024, at 19:57, Aaron Conole wrote:
>
>> Eelco Chaudron <echau...@redhat.com> writes:
>>
>>> On 12 Feb 2024, at 15:15, Aaron Conole wrote:
>>>
>>>> Aaron Conole <acon...@redhat.com> writes:
>>>>
>>>>> Eelco Chaudron <echau...@redhat.com> writes:
>>>>>
>>>>>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
>>>>>>
>>>>>>> On 2/1/24 10:02, Eelco Chaudron wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 31 Jan 2024, at 18:03, Aaron Conole wrote:
>>>>>>>>
>>>>>>>>> Eelco Chaudron <echau...@redhat.com> writes:
>>>>>>>>>
>>>>>>>>>> On 25 Jan 2024, at 21:55, Aaron Conole wrote:
>>>>>>>>>>
>>>>>>>>>>> From: Kevin Sprague <ksprague0...@gmail.com>
>>>>>>>>>>>
>>>>>>>>>>> During normal operations, it is useful to understand when a
>>>>>>>>>>> particular flow
>>>>>>>>>>> gets removed from the system. This can be useful when debugging
>>>>>>>>>>> performance
>>>>>>>>>>> issues tied to ofproto flow changes, trying to determine deployed
>>>>>>>>>>> traffic
>>>>>>>>>>> patterns, or while debugging dynamic systems where ports come and
>>>>>>>>>>> go.
>>>>>>>>>>>
>>>>>>>>>>> Prior to this change, there was a lack of visibility around flow
>>>>>>>>>>> expiration.
>>>>>>>>>>> The existing debugging infrastructure could tell us when a flow was
>>>>>>>>>>> added to
>>>>>>>>>>> the datapath, but not when it was removed or why.
>>>>>>>>>>>
>>>>>>>>>>> This change introduces a USDT probe at the point where the
>>>>>>>>>>> revalidator
>>>>>>>>>>> determines that the flow should be removed. Additionally, we track
>>>>>>>>>>> the
>>>>>>>>>>> reason for the flow eviction and provide that information as well.
>>>>>>>>>>> With
>>>>>>>>>>> this change, we can track the complete flow lifecycle for the
>>>>>>>>>>> netlink
>>>>>>>>>>> datapath by hooking the upcall tracepoint in kernel, the flow put
>>>>>>>>>>> USDT, and
>>>>>>>>>>> the revaldiator USDT, letting us watch as flows are added and
>>>>>>>>>>> removed from
>>>>>>>>>>> the kernel datapath.
>>>>>>>>>>>
>>>>>>>>>>> This change only enables this information via USDT probe, so it
>>>>>>>>>>> won't be
>>>>>>>>>>> possible to access this information any other way (see:
>>>>>>>>>>> Documentation/topics/usdt-probes.rst).
>>>>>>>>>>>
>>>>>>>>>>> Also included is a script
>>>>>>>>>>> (utilities/usdt-scripts/flow_reval_monitor.py)
>>>>>>>>>>> which serves as a demonstration of how the new USDT probe might be
>>>>>>>>>>> used
>>>>>>>>>>> going forward.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Kevin Sprague <ksprague0...@gmail.com>
>>>>>>>>>>> Co-authored-by: Aaron Conole <acon...@redhat.com>
>>>>>>>>>>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>>>>>>>>>>
>>>>>>>>>> Thanks for following this up Aaron! See comments on this patch
>>>>>> below. I have no additional comments on patch 2.
>>>>>>>>>>
>>>>>>>>>> Cheers,
>>>>>>>>>>
>>>>>>>>>> Eelco
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>> Documentation/topics/usdt-probes.rst | 1 +
>>>>>>>>>>> ofproto/ofproto-dpif-upcall.c | 42 +-
>>>>>>>>>>> utilities/automake.mk | 3 +
>>>>>>>>>>> utilities/usdt-scripts/flow_reval_monitor.py | 653
>>>>>>>>>>> +++++++++++++++++++
>>>>>>>>>>> 4 files changed, 693 insertions(+), 6 deletions(-)
>>>>>>>>>>> create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/Documentation/topics/usdt-probes.rst
>>>>>>>>>>> b/Documentation/topics/usdt-probes.rst
>>>>>>>>>>> index e527f43bab..a8da9bb1f7 100644
>>>>>>>>>>> --- a/Documentation/topics/usdt-probes.rst
>>>>>>>>>>> +++ b/Documentation/topics/usdt-probes.rst
>>>>>>>>>>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
>>>>>>>>>>> - dpif_recv:recv_upcall
>>>>>>>>>>> - main:poll_block
>>>>>>>>>>> - main:run_start
>>>>>>>>>>> +- revalidate:flow_result
>>>>>>>>>>> - revalidate_ukey\_\_:entry
>>>>>>>>>>> - revalidate_ukey\_\_:exit
>>>>>>>>>>> - udpif_revalidator:start_dump
>>>>>>>>>>
>>>>>>>>>> You are missing the specific flow_result result section. This is
>>>>>>>>>> from the previous patch:
>>>>>>>>>
>>>>>>>>> D'oh! Thanks for catching it. I'll re-add it.
>>>>>>>>>
>>>>>>>>>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
>>>>>>>>>> - ``utilities/usdt-scripts/bridge_loop.bt``
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> +probe revalidate:flow_result
>>>>>>>>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>>>> +
>>>>>>>>>> +**Description**:
>>>>>>>>>> +This probe is triggered when the revalidator decides whether or not
>>>>>>>>>> to
>>>>>>>>>> +revalidate a flow. ``reason`` is an enum that denotes that either
>>>>>>>>>> the flow
>>>>>>>>>> +is being kept, or the reason why the flow is being deleted. The
>>>>>>>>>> +``flow_reval_monitor.py`` script uses this probe to notify users
>>>>>>>>>> when flows
>>>>>>>>>> +matching user-provided criteria are deleted.
>>>>>>>>>> +
>>>>>>>>>> +**Arguments**:
>>>>>>>>>> +
>>>>>>>>>> +- *arg0*: ``(enum flow_del_reason) reason``
>>>>>>>>>> +- *arg1*: ``(struct udpif *) udpif``
>>>>>>>>>> +- *arg2*: ``(struct udpif_key *) ukey``
>>>>>>>>>> +
>>>>>>>>>> +**Script references**:
>>>>>>>>>> +
>>>>>>>>>> +- ``utilities/usdt-scripts/flow_reval_monitor.py``
>>>>>>>>>> +
>>>>>>>>>> +
>>>>>>>>>> Adding your own probes
>>>>>>>>>> ----------------------
>>>>>>>>>>
>>>>>>>>>>> diff --git a/ofproto/ofproto-dpif-upcall.c
>>>>>>>>>>> b/ofproto/ofproto-dpif-upcall.c
>>>>>>>>>>> index b5cbeed878..97d75833f7 100644
>>>>>>>>>>> --- a/ofproto/ofproto-dpif-upcall.c
>>>>>>>>>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>>>>>>>>>> @@ -269,6 +269,18 @@ enum ukey_state {
>>>>>>>>>>> };
>>>>>>>>>>> #define N_UKEY_STATES (UKEY_DELETED + 1)
>>>>>>>>>>>
>>>>>>>>>>> +enum flow_del_reason {
>>>>>>>>>>> + FDR_REVALIDATE = 0, /* The flow was revalidated. */
>>>>>>>>>>
>>>>>>>>>> It was called FDR_FLOW_LIVE before, which might make more
>>>>>> sense. As the flow is just NOT deleted. It might or might not have
>>>>>> been revalidated. Thoughts?
>>>>>>>>>
>>>>>>>>> I think it had to have been revalidated if we emit the reason, because
>>>>>>>>> we only emit the reason code after revalidation. IE: there are many
>>>>>>>>> places where we skip revalidation but the flow stays live - and we
>>>>>>>>> don't
>>>>>>>>> emit reasons in those cases.
>>>>>>>>>
>>>>>>>>> So at least for this patch, it MUST have been revalidated. But maybe
>>>>>>>>> in
>>>>>>>>> the future, we would want to catch cases where the flow hasn't been.
>>>>>>>>> In
>>>>>>>>> that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
>>>>>>>>> think.
>>>>>>>>>
>>>>>>>>> Maybe you disagree?
>>>>>>>>
>>>>>>>> Well, it depends on how you define revalidation, it might only have
>>>>>> updated the counters. i.e. it all depends on ‘bool need_revalidate =
>>>>>> ukey->reval_seq != reval_seq;’ in revalidate_ukey(). That was why I
>>>>>> opted for a more general name.
>>>>>>>>
>>>>>>>>>>> + FDR_FLOW_IDLE, /* The flow went unused and was
>>>>>>>>>>> deleted. */
>>>>>>>>>>> + FDR_TOO_EXPENSIVE, /* The flow was too expensive to
>>>>>>>>>>> revalidate. */
>>>>>>>>>>> + FDR_FLOW_WILDCARDED, /* The flow needed a narrower wildcard
>>>>>>>>>>> mask. */
>>>>>>>>>>> + FDR_BAD_ODP_FIT, /* The flow had a bad ODP flow fit. */
>>>>>>>>>>> + FDR_NO_OFPROTO, /* The flow didn't have an associated
>>>>>>>>>>> ofproto. */
>>>>>>>>>>> + FDR_XLATION_ERROR, /* There was an error translating the
>>>>>>>>>>> flow. */
>>>>>>>>>>> + FDR_AVOID_CACHING, /* Flow deleted to avoid caching. */
>>>>>>>>>>> + FDR_FLOW_LIMIT, /* All flows being killed. */
>>>>>>>>>>
>>>>>>>>>> Looking at the comment from Han on FDR_PURGE, and this patch
>>>>>> needing another spin, we should probably add it.
>>>>>>>>>
>>>>>>>>> I can do that, sure. In that case, we will need to have a new flow op
>>>>>>>>> added to revalidator_sweep__ so that we can catch it. But in that
>>>>>>>>> case,
>>>>>>>>> it will be a different usdt probe, so I still don't know if we need
>>>>>>>>> FDR_PURGE right? WDYT?
>>>>>>>>
>>>>>>>> In revalidator_sweep__() you have sort of the following:
>>>>>>>>
>>>>>>>> if (purge || ukey_state == UKEY_INCONSISTENT) {
>>>>>>>> result = UKEY_DELETE;
>>>>>>>> } else if (!seq_mismatch) {
>>>>>>>>
>>>>>>>> And I’m afraid that if we use this tool to debug we miss the
>>>>>> ukey_state == UKEY_INCONSISTENT when debugging and spent a long time
>>>>>> figuring this out.
>>>>>>>> Maybe add something general like this (did not give it a lot of
>>>>>> thought), and only take the FDR_PURGE : FDR_UPDATE_FAIL results in the
>>>>>> script?
>>>>>>>>
>>>>>>>> /* 'udpif_key's are responsible for tracking the little bit of state
>>>>>>>> udpif
>>>>>>>> @@ -2991,13 +2993,13 @@ revalidator_sweep__(struct revalidator
>>>>>>>> *revalidator, bool purge)
>>>>>>>> uint64_t odp_actions_stub[1024 / 8];
>>>>>>>> struct ofpbuf odp_actions =
>>>>>>>> OFPBUF_STUB_INITIALIZER(odp_actions_stub);
>>>>>>>>
>>>>>>>> - enum flow_del_reason reason = FDR_REVALIDATE;
>>>>>>>> struct ukey_op ops[REVALIDATE_MAX_BATCH];
>>>>>>>> struct udpif_key *ukey;
>>>>>>>> struct umap *umap = &udpif->ukeys[i];
>>>>>>>> size_t n_ops = 0;
>>>>>>>>
>>>>>>>> CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) {
>>>>>>>> + enum flow_del_reason reason = FDR_REVALIDATE;
>>>>>>>> enum ukey_state ukey_state;
>>>>>>>>
>>>>>>>> /* Handler threads could be holding a ukey lock while it
>>>>>>>> installs a
>>>>>>>> @@ -3016,8 +3018,10 @@ revalidator_sweep__(struct revalidator
>>>>>>>> *revalidator, bool purge)
>>>>>>>>
>>>>>>>> if (purge || ukey_state == UKEY_INCONSISTENT) {
>>>>>>>> result = UKEY_DELETE;
>>>>>>>> + reason = purge ? FDR_PURGE : FDR_UPDATE_FAIL;
>>>>>>>> } else if (!seq_mismatch) {
>>>>>>>> result = UKEY_KEEP;
>>>>>>>> + reason = FDR_REVALIDATE; //_KEEP
>>>>>>>> } else {
>>>>>>>> struct dpif_flow_stats stats;
>>>>>>>> COVERAGE_INC(revalidate_missed_dp_flow);
>>>>>>>> @@ -3030,6 +3034,8 @@ revalidator_sweep__(struct revalidator
>>>>>>>> *revalidator, bool purge)
>>>>>>>> reval_op_init(&ops[n_ops++], result, udpif,
>>>>>>>> ukey, &recircs,
>>>>>>>> &odp_actions);
>>>>>>>> }
>>>>>>>> + OVS_USDT_PROBE(revalidator_sweep__, flow_result,
>>>>>>>> result,
>>>>>>>> + reason, udpif, ukey);
>>>>>>>> }
>>>>>>>> ovs_mutex_unlock(&ukey->mutex);
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> In addition in revalidator_sweep__() should the “enum
>>>>>> flow_del_reason reason = FDR_REVALIDATE;” not be moved to the
>>>>>> CMAP_FOR_EACH() loop?
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> +};
>>>>>>>>>>> +
>>>>>>>>>>> /* 'udpif_key's are responsible for tracking the little bit of
>>>>>>>>>>> state udpif
>>>>>>>>>>> * needs to do flow expiration which can't be pulled directly
>>>>>>>>>>> from the
>>>>>>>>>>> * datapath. They may be created by any handler or revalidator
>>>>>>>>>>> thread at any
>>>>>>>>>>> @@ -2272,7 +2284,8 @@ populate_xcache(struct udpif *udpif, struct
>>>>>>>>>>> udpif_key *ukey,
>>>>>>>>>>> static enum reval_result
>>>>>>>>>>> revalidate_ukey__(struct udpif *udpif, const struct udpif_key
>>>>>>>>>>> *ukey,
>>>>>>>>>>> uint16_t tcp_flags, struct ofpbuf *odp_actions,
>>>>>>>>>>> - struct recirc_refs *recircs, struct xlate_cache
>>>>>>>>>>> *xcache)
>>>>>>>>>>> + struct recirc_refs *recircs, struct xlate_cache
>>>>>>>>>>> *xcache,
>>>>>>>>>>> + enum flow_del_reason *reason)
>>>>>>>>>>> {
>>>>>>>>>>> struct xlate_out *xoutp;
>>>>>>>>>>> struct netflow *netflow;
>>>>>>>>>>> @@ -2293,11 +2306,13 @@ revalidate_ukey__(struct udpif *udpif,
>>>>>> const struct udpif_key *ukey,
>>>>>>>>>>> netflow = NULL;
>>>>>>>>>>>
>>>>>>>>>>> if (xlate_ukey(udpif, ukey, tcp_flags, &ctx)) {
>>>>>>>>>>> + *reason = FDR_XLATION_ERROR;
>>>>>>>>>>> goto exit;
>>>>>>>>>>> }
>>>>>>>>>>> xoutp = &ctx.xout;
>>>>>>>>>>>
>>>>>>>>>>> if (xoutp->avoid_caching) {
>>>>>>>>>>> + *reason = FDR_AVOID_CACHING;
>>>>>>>>>>> goto exit;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> @@ -2311,6 +2326,7 @@ revalidate_ukey__(struct udpif *udpif,
>>>>> const struct udpif_key *ukey,
>>>>>>>>>>> ofpbuf_clear(odp_actions);
>>>>>>>>>>>
>>>>>>>>>>> if (!ofproto) {
>>>>>>>>>>> + *reason = FDR_NO_OFPROTO;
>>>>>>>>>>> goto exit;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> @@ -2322,6 +2338,7 @@ revalidate_ukey__(struct udpif *udpif,
>>>>> const struct udpif_key *ukey,
>>>>>>>>>>> if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len,
>>>>>>>>>>> &dp_mask, &ctx.flow,
>>>>>>>>>>> NULL)
>>>>>>>>>>> == ODP_FIT_ERROR) {
>>>>>>>>>>> + *reason = FDR_BAD_ODP_FIT;
>>>>>>>>>>> goto exit;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> @@ -2331,6 +2348,7 @@ revalidate_ukey__(struct udpif *udpif,
>>>>> const struct udpif_key *ukey,
>>>>>>>>>>> * down. Note that we do not know if the datapath has
>>>>>>>>>>> ignored any of the
>>>>>>>>>>> * wildcarded bits, so we may be overly conservative here. */
>>>>>>>>>>> if (flow_wildcards_has_extra(&dp_mask, ctx.wc)) {
>>>>>>>>>>> + *reason = FDR_FLOW_WILDCARDED;
>>>>>>>>>>> goto exit;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> @@ -2400,7 +2418,7 @@ static enum reval_result
>>>>>>>>>>> revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
>>>>>>>>>>> const struct dpif_flow_stats *stats,
>>>>>>>>>>> struct ofpbuf *odp_actions, uint64_t reval_seq,
>>>>>>>>>>> - struct recirc_refs *recircs)
>>>>>>>>>>> + struct recirc_refs *recircs, enum flow_del_reason
>>>>>>>>>>> *reason)
>>>>>>>>>>> OVS_REQUIRES(ukey->mutex)
>>>>>>>>>>> {
>>>>>>>>>>> bool need_revalidate = ukey->reval_seq != reval_seq;
>>>>>>>>>>> @@ -2430,8 +2448,12 @@ revalidate_ukey(struct udpif *udpif, struct
>>>>>>>>>>> udpif_key *ukey,
>>>>>>>>>>> xlate_cache_clear(ukey->xcache);
>>>>>>>>>>> }
>>>>>>>>>>> result = revalidate_ukey__(udpif, ukey,
>>>>>>>>>>> push.tcp_flags,
>>>>>>>>>>> - odp_actions, recircs,
>>>>>>>>>>> ukey->xcache);
>>>>>>>>>>> - } /* else delete; too expensive to revalidate */
>>>>>>>>>>> + odp_actions, recircs,
>>>>>>>>>>> ukey->xcache,
>>>>>>>>>>> + reason);
>>>>>>>>>>> + } else {
>>>>>>>>>>> + /* delete; too expensive to revalidate */
>>>>>>>>>>> + *reason = FDR_TOO_EXPENSIVE;
>>>>>>>>>>> + }
>>>>>>>>>>> } else if (!push.n_packets || ukey->xcache
>>>>>>>>>>> || !populate_xcache(udpif, ukey, push.tcp_flags)) {
>>>>>>>>>>> result = UKEY_KEEP;
>>>>>>>>>>> @@ -2831,6 +2853,7 @@ revalidate(struct revalidator *revalidator)
>>>>>>>>>>> for (f = flows; f < &flows[n_dumped]; f++) {
>>>>>>>>>>> long long int used = f->stats.used;
>>>>>>>>>>> struct recirc_refs recircs =
>>>>>>>>>>> RECIRC_REFS_EMPTY_INITIALIZER;
>>>>>>>>>>> + enum flow_del_reason reason = FDR_REVALIDATE;
>>>>>>>>>>> struct dpif_flow_stats stats = f->stats;
>>>>>>>>>>> enum reval_result result;
>>>>>>>>>>> struct udpif_key *ukey;
>>>>>>>>>>> @@ -2905,9 +2928,14 @@ revalidate(struct revalidator *revalidator)
>>>>>>>>>>> }
>>>>>>>>>>> if (kill_them_all || (used && used < now - max_idle))
>>>>>>>>>>> {
>>>>>>>>>>> result = UKEY_DELETE;
>>>>>>>>>>> + if (kill_them_all) {
>>>>>>>>>>> + reason = FDR_FLOW_LIMIT;
>>>>>>>>>>> + } else {
>>>>>>>>>>> + reason = FDR_FLOW_IDLE;
>>>>>>>>>>> + }
>>>>>>>>>>> } else {
>>>>>>>>>>> result = revalidate_ukey(udpif, ukey, &stats,
>>>>>>>>>>> &odp_actions,
>>>>>>>>>>> - reval_seq, &recircs);
>>>>>>>>>>> + reval_seq, &recircs,
>>>>>>>>>>> &reason);
>>>>>>>>>>> }
>>>>>>>>>>> ukey->dump_seq = dump_seq;
>>>>>>>>>>>
>>>>>>>>>>> @@ -2916,6 +2944,7 @@ revalidate(struct revalidator *revalidator)
>>>>>>>>>>> udpif_update_flow_pps(udpif, ukey, f);
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> + OVS_USDT_PROBE(revalidate, flow_result, reason, udpif,
>>>>>>>>>>> ukey);
>>>>>>>
>>>>>>> I have been experimenting with several upcall tracking techniques
>>>>>> that would make it easier to correlate upcalls with their subsequent
>>>>>> related events.
>>>>>>> To achieve that, we need (among other things) some easy-to-compare
>>>>>> unique value in the events. For revalidation events, I think a good
>>>>>> candidate would be "ukey->ufid" and so does the script in this patch.
>>>>>>>
>>>>>>> However, requiring all external tools to know the layout of "struct
>>>>>> udpif_key" in order to get that value makes things quite complicated
>>>>>> for CORE tools (e.g: retis).
>>>>>>>
>>>>>>> With all this, would you consider adding the ufid to probe payload
>>>>>>> directly?
>>>>>>
>>>>>> Dont see why we can not, but if we need anything else it would quickly
>>>>>> explode in too much arguments. I guess CORE needs a good solution for
>>>>>> userspace.
>>>>>
>>>>> I think having the ufid and udpif makes sense for now. Actually, maybe
>>>>> for the long term we can do something as an appctl command which will
>>>>> dump the relevant datastructures. That way a tool like this, which
>>>>> needs the pid anyway, can generate an appctl command against the daemon
>>>>> and get the exact right header layout.
>>>>>
>>>>> WDYT?
>>>>
>>>> Update on this - I have it ready to go with the exception of this
>>>> particular change. The USDT probe code generation seems to have a bug,
>>>> or maybe it just won't work using these kinds of data types. For
>>>> example, the bcc tools gets confused and ends up generating code for the
>>>> probe like:
>>>>
>>>> __attribute__((always_inline))
>>>> static __always_inline int _bpf_readarg_usdt__flow_result_3(struct
>>> pt_regs *ctx, void *dest, size_t len) {
>>>> if (len != sizeof(d)) return -1;
>>>> { u64 __addr = ctx->r13 + 40; __asm__ __volatile__("": : :"memory");
>>> uint128_t __res = 0x0; bpf_probe_read_user(&__res, sizeof(__res),
>>> (void *)__addr); *((d *)dest) = __res; }
>>>> return 0;
>>>> }
>>>>
>>>> This code isn't valid - the (d) variable isn't properly represented
>>>> here. I guess maybe there's some issue with the way they use ctypes
>>>> internally? It isn't clear to me.
>>>
>>> I miss some context, but is it because you point to ufid, which is a
>>> union (typedef union ovs_u128)? What if you point to the u32 member of
>>> the union?
>>
>> I'm not sure, but that doesn't seem like the best approach to me, tbh.
>>
>>>> We could work around this by making the ufid always pass as a void *,
>>>> and passing a length that tells the size. However, if the underlying
>>>> data type ever changes, it will be totally incompatible. I doubt it
>>>> would ever change, but at least with something stronger typed, we could
>>>> get a better exception that bubbled up.
>>>
>>> I do not like the pointer + length, as this adds another argument. I
>>> can’t remember which, but some tools had problems with 5 or more
>>> arguments.
>>
>> I do agree. I also want to test the latest version of bcc-tools also,
>> to see if it really is just related to my f38 version of bcc-tools.
>> That also raises a different issue, but it can be something we solve in
>> the interrim with a version check.
>>
>>>> I'm okay with doing it this way, but just wanted to make sure that
>>>> retis, et. al. would be okay with interpreting it this way.
>>>>
>>>> Alternatively, there's definitely a bug there - we could omit the
>>>> argument entirely and rely on pahole or even pyelftools to help extract
>>>> the struct layout and get the ufid from the udpif_key struct. The data
>>>> is already being sent, so it seems like duplication a bit to send it,
>>>> and becomes difficult in light of this bcc "bug?"
>>>
>>> I agree it’s overloading arguments with data already present, but I
>>> guess CORE can not yet read debug data from userspace
>>> applications. Adrian, I thought this was being worked on with the
>>> Pahole guys. If this is close we could just leave out this extra
>>> argument for now.
>>
>> That should be something doable, I think. For example, there are crates
>> like gimli that parse DWARF data (but not sure if it is parsing v5,
>> yet).
>>
>>> Alternatively, we could ask the BCC guys if this is a known issue,
>>> with maybe a potential workaround. Just post it here
>>> https://github.com/iovisor/bcc/issues the respond quickly.
>>
>> They will tell to run with the latest version, I think. The version I'm
>> using is what ships with Fedora 38, and seems quite out of date.
>
> ACK, will wait for your conclusion and the v9.
After tying to get it working with latest bcc-tools, the behavior is
actually much worse. Whereas before the flow_reval_monitor script would
fail during the bpf compilation (with a clear error message), the latest
BCC tools will crash the python interpreter.
I'm going to omit the ufid details. I think CO-RE will have support
anyway for this kinds of output, so v9 I will keep the args as-is and
when the support gets added to decode the debuginfo packages to pull
those structures, they will get the support.
> //Eelco
>
>>> <SNIP>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
