On 7/9/24 16:25, Eelco Chaudron wrote:
>
>
> On 9 Jul 2024, at 16:17, Adrián Moreno wrote:
>
>> On Tue, Jul 09, 2024 at 11:46:12AM GMT, Eelco Chaudron wrote:
>>> On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
>>>
>>>> When sample action gets used as a way of sampling traffic with
>>>> controller-generated metadata (i.e: obs_domain_id and obs_point_id),
>>>> the controller will have to increase the number of flows to ensure each
>>>> part of the pipeline contains the right metadata.
>>>>
>>>> As an example, if the controller decides to sample stateful traffic, it
>>>> could store the computed metadata for each connection in the conntrack
>>>> label. However, for established connections, a flow must be created for
>>>> each different ct_label value with a sample action that contains a
>>>> different hardcoded obs_domain and obs_point id.
>>>>
>>>> This patch adds a new version of the NXAST_RAW_SAMPLE* action (number 4)
>>>> that supports specifying the observation point and domain using an
>>>> OpenFlow field reference, so now the controller can express:
>>>>
>>>> sample(...
>>>> obs_domain_id=NXM_NX_CT_LABEL[0..31],
>>>> obs_point_id=NXM_NX_CT_LABEL[32..63]
>>>> ...
>>>> )
>>>>
>>>> Signed-off-by: Adrian Moreno <[email protected]>
>>>
>>> See some comments inline. I’m missing the documentation update.
>>>
>>
>> Yes. I noticed I missed both the NEWS and documentation update.
>>
You're also missing unit tests in ovs-ofctl.at.
>>>> index 323a58cbf..2aff48f5e 100644
>>>> --- a/ofproto/ofproto-dpif-xlate.c
>>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>>> @@ -5909,6 +5909,44 @@ xlate_fin_timeout(struct xlate_ctx *ctx,
>>>> }
>>>> }
>>>>
>>>> +static uint32_t
>>>> +ofpact_sample_get_domain(struct xlate_ctx *ctx,
>>>> + const struct ofpact_sample *os)
>>>> +{
>>>> + if (os->obs_domain_src.field) {
>>>> + union mf_subvalue *value = xmalloc(sizeof *value);
>>>
>>> Would it be better to just put these 128 bytes on the stack to avoid
>>> memory fragmentation? Same for the next function.
>>>
>>> Or maybe something better, we could do something like we did for
>>> exact_match_mask?
>>>
>>> const union mf_value exact_match_mask = MF_VALUE_EXACT_INITIALIZER;
>>> extern const union mf_value exact_match_mask;
>>>
>>
>> A quick look seemed to indicate dynamic memory was preferred in this
>> module so I assumed there was a reason for it (I do remember stack size
>> being a problem at some point, not sure if it was here).
>>
>> I'll look at the exact_match_mask case.
>
> I guess this function is not called recursively, so we should be ok with the
> stack usage. But a const variable might be better (and we can fix the other
> place(s) where you copied this code from ;).
While the function itself is not called recursively, the problem is
that it can be inlined into the main do_xlate_actions and cause
a significant stack usage increase even if the sampling is not used.
So, extra investigation on what is getting inlined with different
compilers and compiler flags is needed if we want to allocate it
on stack.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
