Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
On 7/11/24 09:28, Adrián Moreno wrote:
> On Thu, Jul 11, 2024 at 12:32:06AM GMT, Ilya Maximets wrote:
>> On 7/10/24 23:38, Adrián Moreno wrote:
>>> On Wed, Jul 10, 2024 at 11:00:43PM GMT, Ilya Maximets wrote:
On 7/7/24 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
> ---
> include/openvswitch/ofp-actions.h | 8 +-
> lib/ofp-actions.c | 249 +++---
> ofproto/ofproto-dpif-xlate.c | 55 ++-
> python/ovs/flow/ofp.py| 8 +-
> python/ovs/flow/ofp_act.py| 4 +-
> tests/ofp-actions.at | 5 +
> tests/ofproto-dpif.at | 41 +
> tests/system-traffic.at | 74 +
> 8 files changed, 405 insertions(+), 39 deletions(-)
Not a full review, it's a complicated change. See a few comments below.
>
> diff --git a/include/openvswitch/ofp-actions.h
> b/include/openvswitch/ofp-actions.h
> index 7b57e49ad..56dc2c147 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
>
> /* OFPACT_SAMPLE.
> *
> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and
> NXAST_SAMPLE4. */
> struct ofpact_sample {
> OFPACT_PADDED_MEMBERS(
> struct ofpact ofpact;
> uint16_t probability; /* Always positive. */
> uint32_t collector_set_id;
> -uint32_t obs_domain_id;
> -uint32_t obs_point_id;
> +uint32_t obs_domain_imm;
> +struct mf_subfield obs_domain_src;
> +uint32_t obs_point_imm;
> +struct mf_subfield obs_point_src;
> ofp_port_t sampling_port;
> enum nx_action_sample_direction direction;
> );
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index da7b1dd31..e329a7e3f 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
> NXAST_RAW_SAMPLE2,
> /* NX1.0+(41): struct nx_action_sample2. */
> NXAST_RAW_SAMPLE3,
> +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
Why the dots are here? The structure doesn't seem to have
extra fields at the end.
>>>
>>> I missread the description then. I thought it was just about alignment.
>>>
> +NXAST_RAW_SAMPLE4,
>
> /* NX1.0+(34): struct nx_action_conjunction. */
> NXAST_RAW_CONJUNCTION,
> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
> };
> OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
>
> +/* Action structure for NXAST_SAMPLE4
> + *
> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to
> NXAST_SAMPLE3,
> + * it adds support for using field specifiers for observation_domain_id
> and
> + * observation_point_id. */
> +struct nx_action_sample4 {
> +ovs_be16 type; /* OFPAT_VENDOR. */
> +ovs_be16 len; /* Length is 32. */
Is the length 32? 40, I suppose.
>>>
>>> Yep
>>>
> +ovs_be32 vendor;/* NX_VENDOR_ID. */
> +ovs_be16 subtype; /* NXAST_SAMPLE. */
NXAST_SAMPLE4 ?
>>>
>>> Ack.
>>>
> +ovs_be16 probability; /* Fraction of packets to sample. */
> +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
> +ovs_be32 obs_domain_src;/* Source of the
> observation_domain_id. */
> +union {
> +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source
> field. */
> +ovs_be32 obs_domain_imm;/* Immediate value for domain
> id. */
> +};
> +ovs_be32 obs_
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
On Thu, Jul 11, 2024 at 12:32:06AM GMT, Ilya Maximets wrote:
> On 7/10/24 23:38, Adrián Moreno wrote:
> > On Wed, Jul 10, 2024 at 11:00:43PM GMT, Ilya Maximets wrote:
> >> On 7/7/24 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
> >>> ---
> >>> include/openvswitch/ofp-actions.h | 8 +-
> >>> lib/ofp-actions.c | 249 +++---
> >>> ofproto/ofproto-dpif-xlate.c | 55 ++-
> >>> python/ovs/flow/ofp.py| 8 +-
> >>> python/ovs/flow/ofp_act.py| 4 +-
> >>> tests/ofp-actions.at | 5 +
> >>> tests/ofproto-dpif.at | 41 +
> >>> tests/system-traffic.at | 74 +
> >>> 8 files changed, 405 insertions(+), 39 deletions(-)
> >>
> >> Not a full review, it's a complicated change. See a few comments below.
> >>
> >>>
> >>> diff --git a/include/openvswitch/ofp-actions.h
> >>> b/include/openvswitch/ofp-actions.h
> >>> index 7b57e49ad..56dc2c147 100644
> >>> --- a/include/openvswitch/ofp-actions.h
> >>> +++ b/include/openvswitch/ofp-actions.h
> >>> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
> >>>
> >>> /* OFPACT_SAMPLE.
> >>> *
> >>> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> >>> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and
> >>> NXAST_SAMPLE4. */
> >>> struct ofpact_sample {
> >>> OFPACT_PADDED_MEMBERS(
> >>> struct ofpact ofpact;
> >>> uint16_t probability; /* Always positive. */
> >>> uint32_t collector_set_id;
> >>> -uint32_t obs_domain_id;
> >>> -uint32_t obs_point_id;
> >>> +uint32_t obs_domain_imm;
> >>> +struct mf_subfield obs_domain_src;
> >>> +uint32_t obs_point_imm;
> >>> +struct mf_subfield obs_point_src;
> >>> ofp_port_t sampling_port;
> >>> enum nx_action_sample_direction direction;
> >>> );
> >>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> >>> index da7b1dd31..e329a7e3f 100644
> >>> --- a/lib/ofp-actions.c
> >>> +++ b/lib/ofp-actions.c
> >>> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
> >>> NXAST_RAW_SAMPLE2,
> >>> /* NX1.0+(41): struct nx_action_sample2. */
> >>> NXAST_RAW_SAMPLE3,
> >>> +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
> >>
> >> Why the dots are here? The structure doesn't seem to have
> >> extra fields at the end.
> >
> > I missread the description then. I thought it was just about alignment.
> >
> >>
> >>> +NXAST_RAW_SAMPLE4,
> >>>
> >>> /* NX1.0+(34): struct nx_action_conjunction. */
> >>> NXAST_RAW_CONJUNCTION,
> >>> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
> >>> };
> >>> OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
> >>>
> >>> +/* Action structure for NXAST_SAMPLE4
> >>> + *
> >>> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to
> >>> NXAST_SAMPLE3,
> >>> + * it adds support for using field specifiers for observation_domain_id
> >>> and
> >>> + * observation_point_id. */
> >>> +struct nx_action_sample4 {
> >>> +ovs_be16 type; /* OFPAT_VENDOR. */
> >>> +ovs_be16 len; /* Length is 32. */
> >>
> >> Is the length 32? 40, I suppose.
> >
> > Yep
> >
> >>
> >>> +ovs_be32 vendor;/* NX_VENDOR_ID. */
> >>> +ovs_be16 subtype; /* NXAST_SAMPLE. */
> >>
> >> NXAST_SAMPLE4 ?
> >
> > Ack.
> >
> >>
> >>> +ovs_be16 probability; /* Fraction of packets to sample. */
> >>> +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
> >>> +ovs_be32 obs_domain_src;/* Source of the
> >>> observation_domain_id. */
> >>> +union {
> >>> +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source
> >>> field. */
> >>> +ovs_be32 obs_domain_imm;/* Immediate value for domain
> >>> id. */
> >>> +};
> >>> +ovs_be32 obs_point_src; /* Source of the
> >>>
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
On 7/10/24 23:38, Adrián Moreno wrote:
> On Wed, Jul 10, 2024 at 11:00:43PM GMT, Ilya Maximets wrote:
>> On 7/7/24 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
>>> ---
>>> include/openvswitch/ofp-actions.h | 8 +-
>>> lib/ofp-actions.c | 249 +++---
>>> ofproto/ofproto-dpif-xlate.c | 55 ++-
>>> python/ovs/flow/ofp.py| 8 +-
>>> python/ovs/flow/ofp_act.py| 4 +-
>>> tests/ofp-actions.at | 5 +
>>> tests/ofproto-dpif.at | 41 +
>>> tests/system-traffic.at | 74 +
>>> 8 files changed, 405 insertions(+), 39 deletions(-)
>>
>> Not a full review, it's a complicated change. See a few comments below.
>>
>>>
>>> diff --git a/include/openvswitch/ofp-actions.h
>>> b/include/openvswitch/ofp-actions.h
>>> index 7b57e49ad..56dc2c147 100644
>>> --- a/include/openvswitch/ofp-actions.h
>>> +++ b/include/openvswitch/ofp-actions.h
>>> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
>>>
>>> /* OFPACT_SAMPLE.
>>> *
>>> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
>>> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4.
>>> */
>>> struct ofpact_sample {
>>> OFPACT_PADDED_MEMBERS(
>>> struct ofpact ofpact;
>>> uint16_t probability; /* Always positive. */
>>> uint32_t collector_set_id;
>>> -uint32_t obs_domain_id;
>>> -uint32_t obs_point_id;
>>> +uint32_t obs_domain_imm;
>>> +struct mf_subfield obs_domain_src;
>>> +uint32_t obs_point_imm;
>>> +struct mf_subfield obs_point_src;
>>> ofp_port_t sampling_port;
>>> enum nx_action_sample_direction direction;
>>> );
>>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
>>> index da7b1dd31..e329a7e3f 100644
>>> --- a/lib/ofp-actions.c
>>> +++ b/lib/ofp-actions.c
>>> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
>>> NXAST_RAW_SAMPLE2,
>>> /* NX1.0+(41): struct nx_action_sample2. */
>>> NXAST_RAW_SAMPLE3,
>>> +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
>>
>> Why the dots are here? The structure doesn't seem to have
>> extra fields at the end.
>
> I missread the description then. I thought it was just about alignment.
>
>>
>>> +NXAST_RAW_SAMPLE4,
>>>
>>> /* NX1.0+(34): struct nx_action_conjunction. */
>>> NXAST_RAW_CONJUNCTION,
>>> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
>>> };
>>> OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
>>>
>>> +/* Action structure for NXAST_SAMPLE4
>>> + *
>>> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to
>>> NXAST_SAMPLE3,
>>> + * it adds support for using field specifiers for observation_domain_id and
>>> + * observation_point_id. */
>>> +struct nx_action_sample4 {
>>> +ovs_be16 type; /* OFPAT_VENDOR. */
>>> +ovs_be16 len; /* Length is 32. */
>>
>> Is the length 32? 40, I suppose.
>
> Yep
>
>>
>>> +ovs_be32 vendor;/* NX_VENDOR_ID. */
>>> +ovs_be16 subtype; /* NXAST_SAMPLE. */
>>
>> NXAST_SAMPLE4 ?
>
> Ack.
>
>>
>>> +ovs_be16 probability; /* Fraction of packets to sample. */
>>> +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
>>> +ovs_be32 obs_domain_src;/* Source of the
>>> observation_domain_id. */
>>> +union {
>>> +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source field.
>>> */
>>> +ovs_be32 obs_domain_imm;/* Immediate value for domain id.
>>> */
>>> +};
>>> +ovs_be32 obs_point_src; /* Source of the observation_point_id
>>> */
>>> +union {
>>> +ovs_be16 obs_point_ofs_nbits; /* Range to use from source field.
>>> */
>>> +ovs_be32 obs_point_imm;/* Immediate value for point id. */
>>> +};
>>> +ovs_be16 sampling_port; /* Sampling port. */
>>> +uint8_t direction
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
On Wed, Jul 10, 2024 at 11:00:43PM GMT, Ilya Maximets wrote:
> On 7/7/24 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
> > ---
> > include/openvswitch/ofp-actions.h | 8 +-
> > lib/ofp-actions.c | 249 +++---
> > ofproto/ofproto-dpif-xlate.c | 55 ++-
> > python/ovs/flow/ofp.py| 8 +-
> > python/ovs/flow/ofp_act.py| 4 +-
> > tests/ofp-actions.at | 5 +
> > tests/ofproto-dpif.at | 41 +
> > tests/system-traffic.at | 74 +
> > 8 files changed, 405 insertions(+), 39 deletions(-)
>
> Not a full review, it's a complicated change. See a few comments below.
>
> >
> > diff --git a/include/openvswitch/ofp-actions.h
> > b/include/openvswitch/ofp-actions.h
> > index 7b57e49ad..56dc2c147 100644
> > --- a/include/openvswitch/ofp-actions.h
> > +++ b/include/openvswitch/ofp-actions.h
> > @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
> >
> > /* OFPACT_SAMPLE.
> > *
> > - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> > + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4.
> > */
> > struct ofpact_sample {
> > OFPACT_PADDED_MEMBERS(
> > struct ofpact ofpact;
> > uint16_t probability; /* Always positive. */
> > uint32_t collector_set_id;
> > -uint32_t obs_domain_id;
> > -uint32_t obs_point_id;
> > +uint32_t obs_domain_imm;
> > +struct mf_subfield obs_domain_src;
> > +uint32_t obs_point_imm;
> > +struct mf_subfield obs_point_src;
> > ofp_port_t sampling_port;
> > enum nx_action_sample_direction direction;
> > );
> > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> > index da7b1dd31..e329a7e3f 100644
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
> > @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
> > NXAST_RAW_SAMPLE2,
> > /* NX1.0+(41): struct nx_action_sample2. */
> > NXAST_RAW_SAMPLE3,
> > +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
>
> Why the dots are here? The structure doesn't seem to have
> extra fields at the end.
I missread the description then. I thought it was just about alignment.
>
> > +NXAST_RAW_SAMPLE4,
> >
> > /* NX1.0+(34): struct nx_action_conjunction. */
> > NXAST_RAW_CONJUNCTION,
> > @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
> > };
> > OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
> >
> > +/* Action structure for NXAST_SAMPLE4
> > + *
> > + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to
> > NXAST_SAMPLE3,
> > + * it adds support for using field specifiers for observation_domain_id and
> > + * observation_point_id. */
> > +struct nx_action_sample4 {
> > +ovs_be16 type; /* OFPAT_VENDOR. */
> > +ovs_be16 len; /* Length is 32. */
>
> Is the length 32? 40, I suppose.
Yep
>
> > +ovs_be32 vendor;/* NX_VENDOR_ID. */
> > +ovs_be16 subtype; /* NXAST_SAMPLE. */
>
> NXAST_SAMPLE4 ?
Ack.
>
> > +ovs_be16 probability; /* Fraction of packets to sample. */
> > +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
> > +ovs_be32 obs_domain_src;/* Source of the
> > observation_domain_id. */
> > +union {
> > +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source field.
> > */
> > +ovs_be32 obs_domain_imm;/* Immediate value for domain id.
> > */
> > +};
> > +ovs_be32 obs_point_src; /* Source of the observation_point_id
> > */
> > +union {
> > +ovs_be16 obs_point_ofs_nbits; /* Range to use from source field.
> > */
> > +ovs_be32 obs_point_imm;/* Immediate value for point id. */
> > +};
> > +ovs_be16 sampling_port; /* Sampling port. */
> > +uint8_t direction; /* Sampling direction. */
> > +uint8_t zeros[5];
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
On 7/7/24 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
> ---
> include/openvswitch/ofp-actions.h | 8 +-
> lib/ofp-actions.c | 249 +++---
> ofproto/ofproto-dpif-xlate.c | 55 ++-
> python/ovs/flow/ofp.py| 8 +-
> python/ovs/flow/ofp_act.py| 4 +-
> tests/ofp-actions.at | 5 +
> tests/ofproto-dpif.at | 41 +
> tests/system-traffic.at | 74 +
> 8 files changed, 405 insertions(+), 39 deletions(-)
Not a full review, it's a complicated change. See a few comments below.
>
> diff --git a/include/openvswitch/ofp-actions.h
> b/include/openvswitch/ofp-actions.h
> index 7b57e49ad..56dc2c147 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
>
> /* OFPACT_SAMPLE.
> *
> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4. */
> struct ofpact_sample {
> OFPACT_PADDED_MEMBERS(
> struct ofpact ofpact;
> uint16_t probability; /* Always positive. */
> uint32_t collector_set_id;
> -uint32_t obs_domain_id;
> -uint32_t obs_point_id;
> +uint32_t obs_domain_imm;
> +struct mf_subfield obs_domain_src;
> +uint32_t obs_point_imm;
> +struct mf_subfield obs_point_src;
> ofp_port_t sampling_port;
> enum nx_action_sample_direction direction;
> );
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index da7b1dd31..e329a7e3f 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
> NXAST_RAW_SAMPLE2,
> /* NX1.0+(41): struct nx_action_sample2. */
> NXAST_RAW_SAMPLE3,
> +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
Why the dots are here? The structure doesn't seem to have
extra fields at the end.
> +NXAST_RAW_SAMPLE4,
>
> /* NX1.0+(34): struct nx_action_conjunction. */
> NXAST_RAW_CONJUNCTION,
> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
> };
> OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
>
> +/* Action structure for NXAST_SAMPLE4
> + *
> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to NXAST_SAMPLE3,
> + * it adds support for using field specifiers for observation_domain_id and
> + * observation_point_id. */
> +struct nx_action_sample4 {
> +ovs_be16 type; /* OFPAT_VENDOR. */
> +ovs_be16 len; /* Length is 32. */
Is the length 32? 40, I suppose.
> +ovs_be32 vendor;/* NX_VENDOR_ID. */
> +ovs_be16 subtype; /* NXAST_SAMPLE. */
NXAST_SAMPLE4 ?
> +ovs_be16 probability; /* Fraction of packets to sample. */
> +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
> +ovs_be32 obs_domain_src;/* Source of the observation_domain_id.
> */
> +union {
> +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source field. */
> +ovs_be32 obs_domain_imm;/* Immediate value for domain id. */
> +};
> +ovs_be32 obs_point_src; /* Source of the observation_point_id */
> +union {
> +ovs_be16 obs_point_ofs_nbits; /* Range to use from source field. */
> +ovs_be32 obs_point_imm;/* Immediate value for point id. */
> +};
> +ovs_be16 sampling_port; /* Sampling port. */
> +uint8_t direction; /* Sampling direction. */
> +uint8_t zeros[5]; /* Pad to a multiple of 8 bytes */
Double spaces are a little strage.
> + };
> + OFP_ASSERT(sizeof(struct nx_action_sample4) == 40);
> +
> static enum ofperr
> decode_NXAST_RAW_SAMPLE(const struct nx_action_sample *nas,
> enum ofp_version ofp_version OVS_UNUSED,
> @@ -6199,11 +6229,14 @@ decode_NXAST_RAW_SAMPLE(const struct nx_action_sample
> *nas,
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
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
> >>>
> >>> 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
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
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
>>>
>>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
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
>>
>> See some comments inline. I’m missing the documentation update.
>>
>
> Yes. I noticed I missed both the NEWS and documentation update.
>
>> Cheers,
>>
>> Eelco
>>
>>> ---
>>> include/openvswitch/ofp-actions.h | 8 +-
>>> lib/ofp-actions.c | 249 +++---
>>> ofproto/ofproto-dpif-xlate.c | 55 ++-
>>> python/ovs/flow/ofp.py| 8 +-
>>> python/ovs/flow/ofp_act.py| 4 +-
>>> tests/ofp-actions.at | 5 +
>>> tests/ofproto-dpif.at | 41 +
>>> tests/system-traffic.at | 74 +
>>> 8 files changed, 405 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/include/openvswitch/ofp-actions.h
>>> b/include/openvswitch/ofp-actions.h
>>> index 7b57e49ad..56dc2c147 100644
>>> --- a/include/openvswitch/ofp-actions.h
>>> +++ b/include/openvswitch/ofp-actions.h
>>> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
>>>
>>> /* OFPACT_SAMPLE.
>>> *
>>> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
>>> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4.
>>> */
>>> struct ofpact_sample {
>>> OFPACT_PADDED_MEMBERS(
>>> struct ofpact ofpact;
>>> uint16_t probability; /* Always positive. */
>>> uint32_t collector_set_id;
>>> -uint32_t obs_domain_id;
>>> -uint32_t obs_point_id;
>>> +uint32_t obs_domain_imm;
>>> +struct mf_subfield obs_domain_src;
>>> +uint32_t obs_point_imm;
>>> +struct mf_subfield obs_point_src;
>>> ofp_port_t sampling_port;
>>> enum nx_action_sample_direction direction;
>>> );
>>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
>>> index da7b1dd31..e329a7e3f 100644
>>> --- a/lib/ofp-actions.c
>>> +++ b/lib/ofp-actions.c
>>> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
>>> NXAST_RAW_SAMPLE2,
>>> /* NX1.0+(41): struct nx_action_sample2. */
>>> NXAST_RAW_SAMPLE3,
>>> +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
>>> +NXAST_RAW_SAMPLE4,
>>>
>>> /* NX1.0+(34): struct nx_action_conjunction. */
>>> NXAST_RAW_CONJUNCTION,
>>> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
>>> };
>>> OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
>>>
>>> +/* Action structure for NXAST_SAMPLE4
>>> + *
>>> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to
>>> NXAST_SAMPLE3,
>>> + * it adds support for using field specifiers for observation_domain_id and
>>> + * observation_point_id. */
>>> +struct nx_action_sample4 {
>>> +ovs_be16 type; /* OFPAT_VENDOR. */
>>> +ovs_be16 len; /* Length is 32. */
>>> +ovs_be32 vendor;/* NX_VENDOR_ID. */
>>> +ovs_be16 subtype; /* NXAST_SAMPLE. */
>>> +ovs_be16 probability; /* Fraction of packets to sample. */
>>> +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
>>> +ovs_be32 obs_domain_src;/* Source of the
>>> observation_domain_id. */
>>> +union {
>>> +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source field.
>>> */
>>> +ovs_be32 obs_domain_imm;/* Immediate value for domain id.
>>> */
>>> +};
>>> +ovs_be32 obs_point_src; /* Source of the observation_point_id
>>> */
>>> +union {
>>> +ovs_be16 obs_point_ofs_nbits; /* Range to use from source field.
>>> */
>>> +ovs_be32 obs_point_imm;/* Immediate value for point id. */
>>> +};
>>> +ovs_be16 sampling_port; /* Sampling port. */
>>> +uint8_t direction; /* Sampling direction. */
>>> +uint8_t zeros[5]; /* Pad to a multiple of 8 bytes */
>>> + };
>>
>> Maybe align all comments, and
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
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
>
> See some comments inline. I’m missing the documentation update.
>
Yes. I noticed I missed both the NEWS and documentation update.
> Cheers,
>
> Eelco
>
> > ---
> > include/openvswitch/ofp-actions.h | 8 +-
> > lib/ofp-actions.c | 249 +++---
> > ofproto/ofproto-dpif-xlate.c | 55 ++-
> > python/ovs/flow/ofp.py| 8 +-
> > python/ovs/flow/ofp_act.py| 4 +-
> > tests/ofp-actions.at | 5 +
> > tests/ofproto-dpif.at | 41 +
> > tests/system-traffic.at | 74 +
> > 8 files changed, 405 insertions(+), 39 deletions(-)
> >
> > diff --git a/include/openvswitch/ofp-actions.h
> > b/include/openvswitch/ofp-actions.h
> > index 7b57e49ad..56dc2c147 100644
> > --- a/include/openvswitch/ofp-actions.h
> > +++ b/include/openvswitch/ofp-actions.h
> > @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
> >
> > /* OFPACT_SAMPLE.
> > *
> > - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> > + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4.
> > */
> > struct ofpact_sample {
> > OFPACT_PADDED_MEMBERS(
> > struct ofpact ofpact;
> > uint16_t probability; /* Always positive. */
> > uint32_t collector_set_id;
> > -uint32_t obs_domain_id;
> > -uint32_t obs_point_id;
> > +uint32_t obs_domain_imm;
> > +struct mf_subfield obs_domain_src;
> > +uint32_t obs_point_imm;
> > +struct mf_subfield obs_point_src;
> > ofp_port_t sampling_port;
> > enum nx_action_sample_direction direction;
> > );
> > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> > index da7b1dd31..e329a7e3f 100644
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
> > @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
> > NXAST_RAW_SAMPLE2,
> > /* NX1.0+(41): struct nx_action_sample2. */
> > NXAST_RAW_SAMPLE3,
> > +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
> > +NXAST_RAW_SAMPLE4,
> >
> > /* NX1.0+(34): struct nx_action_conjunction. */
> > NXAST_RAW_CONJUNCTION,
> > @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
> > };
> > OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
> >
> > +/* Action structure for NXAST_SAMPLE4
> > + *
> > + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to
> > NXAST_SAMPLE3,
> > + * it adds support for using field specifiers for observation_domain_id and
> > + * observation_point_id. */
> > +struct nx_action_sample4 {
> > +ovs_be16 type; /* OFPAT_VENDOR. */
> > +ovs_be16 len; /* Length is 32. */
> > +ovs_be32 vendor;/* NX_VENDOR_ID. */
> > +ovs_be16 subtype; /* NXAST_SAMPLE. */
> > +ovs_be16 probability; /* Fraction of packets to sample. */
> > +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
> > +ovs_be32 obs_domain_src;/* Source of the
> > observation_domain_id. */
> > +union {
> > +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source field.
> > */
> > +ovs_be32 obs_domain_imm;/* Immediate value for domain id.
> > */
> > +};
> > +ovs_be32 obs_point_src; /* Source of the observation_point_id
> > */
> > +union {
> > +ovs_be16 obs_point_ofs_nbits; /* Range to use from source field.
> > */
> > +ovs_be32 obs_point_imm;/* Immediate value for point id. */
> > +};
> > +ovs_be16 sampling_port; /* Sampling port. */
> > +uint8_t direction; /* Sampling direction. */
> > +uint8_t zeros[5]; /* Pad to a multiple of 8 bytes */
> > + };
>
> Maybe align all comments, and make sure all end with a dot.
>
> struct nx_action_sample4 {
>
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
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
See some comments inline. I’m missing the documentation update.
Cheers,
Eelco
> ---
> include/openvswitch/ofp-actions.h | 8 +-
> lib/ofp-actions.c | 249 +++---
> ofproto/ofproto-dpif-xlate.c | 55 ++-
> python/ovs/flow/ofp.py| 8 +-
> python/ovs/flow/ofp_act.py| 4 +-
> tests/ofp-actions.at | 5 +
> tests/ofproto-dpif.at | 41 +
> tests/system-traffic.at | 74 +
> 8 files changed, 405 insertions(+), 39 deletions(-)
>
> diff --git a/include/openvswitch/ofp-actions.h
> b/include/openvswitch/ofp-actions.h
> index 7b57e49ad..56dc2c147 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
>
> /* OFPACT_SAMPLE.
> *
> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4. */
> struct ofpact_sample {
> OFPACT_PADDED_MEMBERS(
> struct ofpact ofpact;
> uint16_t probability; /* Always positive. */
> uint32_t collector_set_id;
> -uint32_t obs_domain_id;
> -uint32_t obs_point_id;
> +uint32_t obs_domain_imm;
> +struct mf_subfield obs_domain_src;
> +uint32_t obs_point_imm;
> +struct mf_subfield obs_point_src;
> ofp_port_t sampling_port;
> enum nx_action_sample_direction direction;
> );
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index da7b1dd31..e329a7e3f 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
> NXAST_RAW_SAMPLE2,
> /* NX1.0+(41): struct nx_action_sample2. */
> NXAST_RAW_SAMPLE3,
> +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
> +NXAST_RAW_SAMPLE4,
>
> /* NX1.0+(34): struct nx_action_conjunction. */
> NXAST_RAW_CONJUNCTION,
> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
> };
> OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
>
> +/* Action structure for NXAST_SAMPLE4
> + *
> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to NXAST_SAMPLE3,
> + * it adds support for using field specifiers for observation_domain_id and
> + * observation_point_id. */
> +struct nx_action_sample4 {
> +ovs_be16 type; /* OFPAT_VENDOR. */
> +ovs_be16 len; /* Length is 32. */
> +ovs_be32 vendor;/* NX_VENDOR_ID. */
> +ovs_be16 subtype; /* NXAST_SAMPLE. */
> +ovs_be16 probability; /* Fraction of packets to sample. */
> +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
> +ovs_be32 obs_domain_src;/* Source of the observation_domain_id.
> */
> +union {
> +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source field. */
> +ovs_be32 obs_domain_imm;/* Immediate value for domain id. */
> +};
> +ovs_be32 obs_point_src; /* Source of the observation_point_id */
> +union {
> +ovs_be16 obs_point_ofs_nbits; /* Range to use from source field. */
> +ovs_be32 obs_point_imm;/* Immediate value for point id. */
> +};
> +ovs_be16 sampling_port; /* Sampling port. */
> +uint8_t direction; /* Sampling direction. */
> +uint8_t zeros[5]; /* Pad to a multiple of 8 bytes */
> + };
Maybe align all comments, and make sure all end with a dot.
struct nx_action_sample4 {
ovs_be16 type; /* OFPAT_VENDOR. */
ovs_be16 len; /* Length is 32. */
ovs_be32 vendor; /* NX_VENDOR_ID. */
ovs_be16 subtype; /* NXAST_SAMPLE. */
ovs_be16 probability; /* Fraction of packets to sample. */
ovs_be32 collector_set_id; /* ID of collector set in OVSDB. *
[ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
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
---
include/openvswitch/ofp-actions.h | 8 +-
lib/ofp-actions.c | 249 +++---
ofproto/ofproto-dpif-xlate.c | 55 ++-
python/ovs/flow/ofp.py| 8 +-
python/ovs/flow/ofp_act.py| 4 +-
tests/ofp-actions.at | 5 +
tests/ofproto-dpif.at | 41 +
tests/system-traffic.at | 74 +
8 files changed, 405 insertions(+), 39 deletions(-)
diff --git a/include/openvswitch/ofp-actions.h
b/include/openvswitch/ofp-actions.h
index 7b57e49ad..56dc2c147 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
/* OFPACT_SAMPLE.
*
- * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
+ * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4. */
struct ofpact_sample {
OFPACT_PADDED_MEMBERS(
struct ofpact ofpact;
uint16_t probability; /* Always positive. */
uint32_t collector_set_id;
-uint32_t obs_domain_id;
-uint32_t obs_point_id;
+uint32_t obs_domain_imm;
+struct mf_subfield obs_domain_src;
+uint32_t obs_point_imm;
+struct mf_subfield obs_point_src;
ofp_port_t sampling_port;
enum nx_action_sample_direction direction;
);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index da7b1dd31..e329a7e3f 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -330,6 +330,8 @@ enum ofp_raw_action_type {
NXAST_RAW_SAMPLE2,
/* NX1.0+(41): struct nx_action_sample2. */
NXAST_RAW_SAMPLE3,
+/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
+NXAST_RAW_SAMPLE4,
/* NX1.0+(34): struct nx_action_conjunction. */
NXAST_RAW_CONJUNCTION,
@@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
};
OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
+/* Action structure for NXAST_SAMPLE4
+ *
+ * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to NXAST_SAMPLE3,
+ * it adds support for using field specifiers for observation_domain_id and
+ * observation_point_id. */
+struct nx_action_sample4 {
+ovs_be16 type; /* OFPAT_VENDOR. */
+ovs_be16 len; /* Length is 32. */
+ovs_be32 vendor;/* NX_VENDOR_ID. */
+ovs_be16 subtype; /* NXAST_SAMPLE. */
+ovs_be16 probability; /* Fraction of packets to sample. */
+ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
+ovs_be32 obs_domain_src;/* Source of the observation_domain_id. */
+union {
+ovs_be16 obs_domain_ofs_nbits; /* Range to use from source field. */
+ovs_be32 obs_domain_imm;/* Immediate value for domain id. */
+};
+ovs_be32 obs_point_src; /* Source of the observation_point_id */
+union {
+ovs_be16 obs_point_ofs_nbits; /* Range to use from source field. */
+ovs_be32 obs_point_imm;/* Immediate value for point id. */
+};
+ovs_be16 sampling_port; /* Sampling port. */
+uint8_t direction; /* Sampling direction. */
+uint8_t zeros[5]; /* Pad to a multiple of 8 bytes */
+ };
+ OFP_ASSERT(sizeof(struct nx_action_sample4) == 40);
+
static enum ofperr
decode_NXAST_RAW_SAMPLE(const struct nx_action_sample *nas,
enum ofp_version ofp_version OVS_UNUSED,
@@ -6199,11 +6229,14 @@ decode_NXAST_RAW_SAMPLE(const struct nx_action_sample
*nas,
sample->ofpact.raw = NXAST_RAW_SAMPLE;
sample->probability = ntohs(nas->probability);
sample->collector_set_id = ntohl(nas->collector_set_id);
-sample->obs_domain_id = ntohl(nas->obs_domain_id);
-sample->obs_point_id = ntohl(nas->obs_point_id);
+sample->obs_domain_imm = ntohl(nas->obs_domain_id);
+sample->obs_domain_src.field = NULL;
+sample->obs_point_imm = ntohl(nas->obs_point_id);
+sample->obs_point_src.field = NULL;
sample->sampling_port = OFPP_NONE;
s
