On Wed, Jul 10, 2024 at 10:26:23PM GMT, Ilya Maximets wrote:
> 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 <amore...@redhat.com>
> >>>
> >>> 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.
>
Ack.
> >>>> 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.
>
For the next version, I have implemented Eelco's suggestion: an extern
constant all-one enum mf_subfield. That should avoid both stack increase
and memory fragmentation
Cheers,
Adrián
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
