Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On 1 Jul 2024, at 14:22, Adrián Moreno wrote:
> On Mon, Jul 01, 2024 at 01:30:12PM GMT, Eelco Chaudron wrote:
>>
>>
>> On 28 Jun 2024, at 18:40, Adrián Moreno wrote:
>>
>>> On Wed, Jun 26, 2024 at 02:11:51PM GMT, Eelco Chaudron wrote:
On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> Use the newly added emit_sample to implement OpenFlow sample() actions
> with local sampling configuration.
See some comments below.
Cheers,
Eelco
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif-lsample.c | 17
> ofproto/ofproto-dpif-lsample.h | 6 ++
> ofproto/ofproto-dpif-xlate.c | 163 -
> ofproto/ofproto-dpif-xlate.h | 3 +-
> ofproto/ofproto-dpif.c | 2 +-
> 5 files changed, 144 insertions(+), 47 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-lsample.c
> b/ofproto/ofproto-dpif-lsample.c
> index 7bdafac19..2c0b5da89 100644
> --- a/ofproto/ofproto-dpif-lsample.c
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -139,6 +139,23 @@ dpif_lsample_set_options(struct dpif_lsample
> *lsample,
> return changed;
> }
>
> +/* Returns the group_id of the exporter with the given collector_set_id,
> if it
> + * exists. */
nit: The below will fit on one line
/* Returns the exporter group_id for a given collector_set_id, if it
exists. */
>>>
>>> Ack.
>>>
> +bool
> +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t
> collector_set_id,
> + uint32_t *group_id)
> +{
> +struct lsample_exporter_node *node;
> +bool found = false;
> +
> +node = dpif_lsample_find_exporter_node(ps, collector_set_id);
> +if (node) {
> +found = true;
> +*group_id = node->exporter.options.group_id;
> +}
> +return found;
> +}
> +
> struct dpif_lsample *
> dpif_lsample_create(void)
> {
> diff --git a/ofproto/ofproto-dpif-lsample.h
> b/ofproto/ofproto-dpif-lsample.h
> index c23ea8372..f13425575 100644
> --- a/ofproto/ofproto-dpif-lsample.h
> +++ b/ofproto/ofproto-dpif-lsample.h
> @@ -19,6 +19,7 @@
>
> #include
> #include
> +#include
Maybe add in alphabetical order.
>>>
>>> Ack.
>>>
> struct dpif_lsample;
> struct ofproto_lsample_options;
> @@ -31,4 +32,9 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
>const struct ofproto_lsample_options *,
>size_t n_opts);
>
> +bool dpif_lsample_get_group_id(struct dpif_lsample *,
> + uint32_t collector_set_id,
> + uint32_t *group_id);
> +
> #endif /* OFPROTO_DPIF_LSAMPLE_H */
> +
Accedantely added a newline?
>>>
>>> Ack.
>>>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 7c4950895..5bd215d31 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -47,6 +47,7 @@
> #include "ofproto/ofproto-dpif-ipfix.h"
> #include "ofproto/ofproto-dpif-mirror.h"
> #include "ofproto/ofproto-dpif-monitor.h"
> +#include "ofproto/ofproto-dpif-lsample.h"
> #include "ofproto/ofproto-dpif-sflow.h"
> #include "ofproto/ofproto-dpif-trace.h"
> #include "ofproto/ofproto-dpif-xlate-cache.h"
> @@ -117,6 +118,7 @@ struct xbridge {
> struct dpif_sflow *sflow; /* SFlow handle, or null. */
> struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
> struct netflow *netflow; /* Netflow handle, or null. */
> +struct dpif_lsample *lsample; /* Local sample handle, or null. */
> struct stp *stp; /* STP or null if disabled. */
> struct rstp *rstp;/* RSTP or null if disabled. */
>
> @@ -687,6 +689,7 @@ static void xlate_xbridge_set(struct xbridge *,
> struct dpif *,
>const struct dpif_sflow *,
>const struct dpif_ipfix *,
>const struct netflow *,
> + const struct dpif_lsample *,
>bool forward_bpdu, bool has_in_band,
>const struct dpif_backer_support *,
>const struct xbridge_addr *);
> @@ -1070,6 +1073,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
>const struct dpif_sflow *sflow,
>const struct dpif_ipfix *ipfix,
>const struct netflow *netflow,
> + const struct dpif_lsample *lsample,
nit: I would move above netflow, as you also do the actual init/unref
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On Mon, Jul 01, 2024 at 01:30:12PM GMT, Eelco Chaudron wrote:
>
>
> On 28 Jun 2024, at 18:40, Adrián Moreno wrote:
>
> > On Wed, Jun 26, 2024 at 02:11:51PM GMT, Eelco Chaudron wrote:
> >> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> >>
> >>> Use the newly added emit_sample to implement OpenFlow sample() actions
> >>> with local sampling configuration.
> >>
> >> See some comments below.
> >>
> >> Cheers,
> >>
> >> Eelco
> >>
> >>> Signed-off-by: Adrian Moreno
> >>> ---
> >>> ofproto/ofproto-dpif-lsample.c | 17
> >>> ofproto/ofproto-dpif-lsample.h | 6 ++
> >>> ofproto/ofproto-dpif-xlate.c | 163 -
> >>> ofproto/ofproto-dpif-xlate.h | 3 +-
> >>> ofproto/ofproto-dpif.c | 2 +-
> >>> 5 files changed, 144 insertions(+), 47 deletions(-)
> >>>
> >>> diff --git a/ofproto/ofproto-dpif-lsample.c
> >>> b/ofproto/ofproto-dpif-lsample.c
> >>> index 7bdafac19..2c0b5da89 100644
> >>> --- a/ofproto/ofproto-dpif-lsample.c
> >>> +++ b/ofproto/ofproto-dpif-lsample.c
> >>> @@ -139,6 +139,23 @@ dpif_lsample_set_options(struct dpif_lsample
> >>> *lsample,
> >>> return changed;
> >>> }
> >>>
> >>> +/* Returns the group_id of the exporter with the given collector_set_id,
> >>> if it
> >>> + * exists. */
> >>
> >> nit: The below will fit on one line
> >>
> >> /* Returns the exporter group_id for a given collector_set_id, if it
> >> exists. */
> >>
> >
> > Ack.
> >
> >>> +bool
> >>> +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t
> >>> collector_set_id,
> >>> + uint32_t *group_id)
> >>> +{
> >>> +struct lsample_exporter_node *node;
> >>> +bool found = false;
> >>> +
> >>> +node = dpif_lsample_find_exporter_node(ps, collector_set_id);
> >>> +if (node) {
> >>> +found = true;
> >>> +*group_id = node->exporter.options.group_id;
> >>> +}
> >>> +return found;
> >>> +}
> >>> +
> >>> struct dpif_lsample *
> >>> dpif_lsample_create(void)
> >>> {
> >>> diff --git a/ofproto/ofproto-dpif-lsample.h
> >>> b/ofproto/ofproto-dpif-lsample.h
> >>> index c23ea8372..f13425575 100644
> >>> --- a/ofproto/ofproto-dpif-lsample.h
> >>> +++ b/ofproto/ofproto-dpif-lsample.h
> >>> @@ -19,6 +19,7 @@
> >>>
> >>> #include
> >>> #include
> >>> +#include
> >>
> >> Maybe add in alphabetical order.
> >>
> >
> > Ack.
> >
> >>> struct dpif_lsample;
> >>> struct ofproto_lsample_options;
> >>> @@ -31,4 +32,9 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
> >>>const struct ofproto_lsample_options *,
> >>>size_t n_opts);
> >>>
> >>> +bool dpif_lsample_get_group_id(struct dpif_lsample *,
> >>> + uint32_t collector_set_id,
> >>> + uint32_t *group_id);
> >>> +
> >>> #endif /* OFPROTO_DPIF_LSAMPLE_H */
> >>> +
> >>
> >> Accedantely added a newline?
> >>
> >
> > Ack.
> >
> >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> >>> index 7c4950895..5bd215d31 100644
> >>> --- a/ofproto/ofproto-dpif-xlate.c
> >>> +++ b/ofproto/ofproto-dpif-xlate.c
> >>> @@ -47,6 +47,7 @@
> >>> #include "ofproto/ofproto-dpif-ipfix.h"
> >>> #include "ofproto/ofproto-dpif-mirror.h"
> >>> #include "ofproto/ofproto-dpif-monitor.h"
> >>> +#include "ofproto/ofproto-dpif-lsample.h"
> >>> #include "ofproto/ofproto-dpif-sflow.h"
> >>> #include "ofproto/ofproto-dpif-trace.h"
> >>> #include "ofproto/ofproto-dpif-xlate-cache.h"
> >>> @@ -117,6 +118,7 @@ struct xbridge {
> >>> struct dpif_sflow *sflow; /* SFlow handle, or null. */
> >>> struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
> >>> struct netflow *netflow; /* Netflow handle, or null. */
> >>> +struct dpif_lsample *lsample; /* Local sample handle, or null. */
> >>> struct stp *stp; /* STP or null if disabled. */
> >>> struct rstp *rstp;/* RSTP or null if disabled. */
> >>>
> >>> @@ -687,6 +689,7 @@ static void xlate_xbridge_set(struct xbridge *,
> >>> struct dpif *,
> >>>const struct dpif_sflow *,
> >>>const struct dpif_ipfix *,
> >>>const struct netflow *,
> >>> + const struct dpif_lsample *,
> >>>bool forward_bpdu, bool has_in_band,
> >>>const struct dpif_backer_support *,
> >>>const struct xbridge_addr *);
> >>> @@ -1070,6 +1073,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
> >>>const struct dpif_sflow *sflow,
> >>>const struct dpif_ipfix *ipfix,
> >>>const struct netflow *netflow,
> >>> + const struct dpif_lsample *lsample,
> >>
> >> nit: I would move above netflow, as you also do the actual init/unref
> >> before
> >> netflow.
> >
> > Ack.
> >
> >>>
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On 28 Jun 2024, at 18:40, Adrián Moreno wrote:
> On Wed, Jun 26, 2024 at 02:11:51PM GMT, Eelco Chaudron wrote:
>> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>>
>>> Use the newly added emit_sample to implement OpenFlow sample() actions
>>> with local sampling configuration.
>>
>> See some comments below.
>>
>> Cheers,
>>
>> Eelco
>>
>>> Signed-off-by: Adrian Moreno
>>> ---
>>> ofproto/ofproto-dpif-lsample.c | 17
>>> ofproto/ofproto-dpif-lsample.h | 6 ++
>>> ofproto/ofproto-dpif-xlate.c | 163 -
>>> ofproto/ofproto-dpif-xlate.h | 3 +-
>>> ofproto/ofproto-dpif.c | 2 +-
>>> 5 files changed, 144 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
>>> index 7bdafac19..2c0b5da89 100644
>>> --- a/ofproto/ofproto-dpif-lsample.c
>>> +++ b/ofproto/ofproto-dpif-lsample.c
>>> @@ -139,6 +139,23 @@ dpif_lsample_set_options(struct dpif_lsample *lsample,
>>> return changed;
>>> }
>>>
>>> +/* Returns the group_id of the exporter with the given collector_set_id,
>>> if it
>>> + * exists. */
>>
>> nit: The below will fit on one line
>>
>> /* Returns the exporter group_id for a given collector_set_id, if it exists.
>> */
>>
>
> Ack.
>
>>> +bool
>>> +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t
>>> collector_set_id,
>>> + uint32_t *group_id)
>>> +{
>>> +struct lsample_exporter_node *node;
>>> +bool found = false;
>>> +
>>> +node = dpif_lsample_find_exporter_node(ps, collector_set_id);
>>> +if (node) {
>>> +found = true;
>>> +*group_id = node->exporter.options.group_id;
>>> +}
>>> +return found;
>>> +}
>>> +
>>> struct dpif_lsample *
>>> dpif_lsample_create(void)
>>> {
>>> diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
>>> index c23ea8372..f13425575 100644
>>> --- a/ofproto/ofproto-dpif-lsample.h
>>> +++ b/ofproto/ofproto-dpif-lsample.h
>>> @@ -19,6 +19,7 @@
>>>
>>> #include
>>> #include
>>> +#include
>>
>> Maybe add in alphabetical order.
>>
>
> Ack.
>
>>> struct dpif_lsample;
>>> struct ofproto_lsample_options;
>>> @@ -31,4 +32,9 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
>>>const struct ofproto_lsample_options *,
>>>size_t n_opts);
>>>
>>> +bool dpif_lsample_get_group_id(struct dpif_lsample *,
>>> + uint32_t collector_set_id,
>>> + uint32_t *group_id);
>>> +
>>> #endif /* OFPROTO_DPIF_LSAMPLE_H */
>>> +
>>
>> Accedantely added a newline?
>>
>
> Ack.
>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index 7c4950895..5bd215d31 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -47,6 +47,7 @@
>>> #include "ofproto/ofproto-dpif-ipfix.h"
>>> #include "ofproto/ofproto-dpif-mirror.h"
>>> #include "ofproto/ofproto-dpif-monitor.h"
>>> +#include "ofproto/ofproto-dpif-lsample.h"
>>> #include "ofproto/ofproto-dpif-sflow.h"
>>> #include "ofproto/ofproto-dpif-trace.h"
>>> #include "ofproto/ofproto-dpif-xlate-cache.h"
>>> @@ -117,6 +118,7 @@ struct xbridge {
>>> struct dpif_sflow *sflow; /* SFlow handle, or null. */
>>> struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
>>> struct netflow *netflow; /* Netflow handle, or null. */
>>> +struct dpif_lsample *lsample; /* Local sample handle, or null. */
>>> struct stp *stp; /* STP or null if disabled. */
>>> struct rstp *rstp;/* RSTP or null if disabled. */
>>>
>>> @@ -687,6 +689,7 @@ static void xlate_xbridge_set(struct xbridge *, struct
>>> dpif *,
>>>const struct dpif_sflow *,
>>>const struct dpif_ipfix *,
>>>const struct netflow *,
>>> + const struct dpif_lsample *,
>>>bool forward_bpdu, bool has_in_band,
>>>const struct dpif_backer_support *,
>>>const struct xbridge_addr *);
>>> @@ -1070,6 +1073,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
>>>const struct dpif_sflow *sflow,
>>>const struct dpif_ipfix *ipfix,
>>>const struct netflow *netflow,
>>> + const struct dpif_lsample *lsample,
>>
>> nit: I would move above netflow, as you also do the actual init/unref before
>> netflow.
>
> Ack.
>
>>>bool forward_bpdu, bool has_in_band,
>>>const struct dpif_backer_support *support,
>>>const struct xbridge_addr *addr)
>>> @@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge,
>>> xbridge->ipfix = dpif_ipfix_ref(ipfix);
>>> }
>>>
>>> +if (xbridge->lsample != lsample) {
>>> +
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On Wed, Jun 26, 2024 at 02:11:51PM GMT, Eelco Chaudron wrote:
> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>
> > Use the newly added emit_sample to implement OpenFlow sample() actions
> > with local sampling configuration.
>
> See some comments below.
>
> Cheers,
>
> Eelco
>
> > Signed-off-by: Adrian Moreno
> > ---
> > ofproto/ofproto-dpif-lsample.c | 17
> > ofproto/ofproto-dpif-lsample.h | 6 ++
> > ofproto/ofproto-dpif-xlate.c | 163 -
> > ofproto/ofproto-dpif-xlate.h | 3 +-
> > ofproto/ofproto-dpif.c | 2 +-
> > 5 files changed, 144 insertions(+), 47 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> > index 7bdafac19..2c0b5da89 100644
> > --- a/ofproto/ofproto-dpif-lsample.c
> > +++ b/ofproto/ofproto-dpif-lsample.c
> > @@ -139,6 +139,23 @@ dpif_lsample_set_options(struct dpif_lsample *lsample,
> > return changed;
> > }
> >
> > +/* Returns the group_id of the exporter with the given collector_set_id,
> > if it
> > + * exists. */
>
> nit: The below will fit on one line
>
> /* Returns the exporter group_id for a given collector_set_id, if it exists.
> */
>
Ack.
> > +bool
> > +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t
> > collector_set_id,
> > + uint32_t *group_id)
> > +{
> > +struct lsample_exporter_node *node;
> > +bool found = false;
> > +
> > +node = dpif_lsample_find_exporter_node(ps, collector_set_id);
> > +if (node) {
> > +found = true;
> > +*group_id = node->exporter.options.group_id;
> > +}
> > +return found;
> > +}
> > +
> > struct dpif_lsample *
> > dpif_lsample_create(void)
> > {
> > diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
> > index c23ea8372..f13425575 100644
> > --- a/ofproto/ofproto-dpif-lsample.h
> > +++ b/ofproto/ofproto-dpif-lsample.h
> > @@ -19,6 +19,7 @@
> >
> > #include
> > #include
> > +#include
>
> Maybe add in alphabetical order.
>
Ack.
> > struct dpif_lsample;
> > struct ofproto_lsample_options;
> > @@ -31,4 +32,9 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
> >const struct ofproto_lsample_options *,
> >size_t n_opts);
> >
> > +bool dpif_lsample_get_group_id(struct dpif_lsample *,
> > + uint32_t collector_set_id,
> > + uint32_t *group_id);
> > +
> > #endif /* OFPROTO_DPIF_LSAMPLE_H */
> > +
>
> Accedantely added a newline?
>
Ack.
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 7c4950895..5bd215d31 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -47,6 +47,7 @@
> > #include "ofproto/ofproto-dpif-ipfix.h"
> > #include "ofproto/ofproto-dpif-mirror.h"
> > #include "ofproto/ofproto-dpif-monitor.h"
> > +#include "ofproto/ofproto-dpif-lsample.h"
> > #include "ofproto/ofproto-dpif-sflow.h"
> > #include "ofproto/ofproto-dpif-trace.h"
> > #include "ofproto/ofproto-dpif-xlate-cache.h"
> > @@ -117,6 +118,7 @@ struct xbridge {
> > struct dpif_sflow *sflow; /* SFlow handle, or null. */
> > struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
> > struct netflow *netflow; /* Netflow handle, or null. */
> > +struct dpif_lsample *lsample; /* Local sample handle, or null. */
> > struct stp *stp; /* STP or null if disabled. */
> > struct rstp *rstp;/* RSTP or null if disabled. */
> >
> > @@ -687,6 +689,7 @@ static void xlate_xbridge_set(struct xbridge *, struct
> > dpif *,
> >const struct dpif_sflow *,
> >const struct dpif_ipfix *,
> >const struct netflow *,
> > + const struct dpif_lsample *,
> >bool forward_bpdu, bool has_in_band,
> >const struct dpif_backer_support *,
> >const struct xbridge_addr *);
> > @@ -1070,6 +1073,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
> >const struct dpif_sflow *sflow,
> >const struct dpif_ipfix *ipfix,
> >const struct netflow *netflow,
> > + const struct dpif_lsample *lsample,
>
> nit: I would move above netflow, as you also do the actual init/unref before
> netflow.
Ack.
> >bool forward_bpdu, bool has_in_band,
> >const struct dpif_backer_support *support,
> >const struct xbridge_addr *addr)
> > @@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge,
> > xbridge->ipfix = dpif_ipfix_ref(ipfix);
> > }
> >
> > +if (xbridge->lsample != lsample) {
> > +dpif_lsample_unref(xbridge->lsample);
> > +xbridge->lsample =
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> Use the newly added emit_sample to implement OpenFlow sample() actions
> with local sampling configuration.
See some comments below.
Cheers,
Eelco
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif-lsample.c | 17
> ofproto/ofproto-dpif-lsample.h | 6 ++
> ofproto/ofproto-dpif-xlate.c | 163 -
> ofproto/ofproto-dpif-xlate.h | 3 +-
> ofproto/ofproto-dpif.c | 2 +-
> 5 files changed, 144 insertions(+), 47 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> index 7bdafac19..2c0b5da89 100644
> --- a/ofproto/ofproto-dpif-lsample.c
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -139,6 +139,23 @@ dpif_lsample_set_options(struct dpif_lsample *lsample,
> return changed;
> }
>
> +/* Returns the group_id of the exporter with the given collector_set_id, if
> it
> + * exists. */
nit: The below will fit on one line
/* Returns the exporter group_id for a given collector_set_id, if it exists. */
> +bool
> +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t collector_set_id,
> + uint32_t *group_id)
> +{
> +struct lsample_exporter_node *node;
> +bool found = false;
> +
> +node = dpif_lsample_find_exporter_node(ps, collector_set_id);
> +if (node) {
> +found = true;
> +*group_id = node->exporter.options.group_id;
> +}
> +return found;
> +}
> +
> struct dpif_lsample *
> dpif_lsample_create(void)
> {
> diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
> index c23ea8372..f13425575 100644
> --- a/ofproto/ofproto-dpif-lsample.h
> +++ b/ofproto/ofproto-dpif-lsample.h
> @@ -19,6 +19,7 @@
>
> #include
> #include
> +#include
Maybe add in alphabetical order.
> struct dpif_lsample;
> struct ofproto_lsample_options;
> @@ -31,4 +32,9 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
>const struct ofproto_lsample_options *,
>size_t n_opts);
>
> +bool dpif_lsample_get_group_id(struct dpif_lsample *,
> + uint32_t collector_set_id,
> + uint32_t *group_id);
> +
> #endif /* OFPROTO_DPIF_LSAMPLE_H */
> +
Accedantely added a newline?
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 7c4950895..5bd215d31 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -47,6 +47,7 @@
> #include "ofproto/ofproto-dpif-ipfix.h"
> #include "ofproto/ofproto-dpif-mirror.h"
> #include "ofproto/ofproto-dpif-monitor.h"
> +#include "ofproto/ofproto-dpif-lsample.h"
> #include "ofproto/ofproto-dpif-sflow.h"
> #include "ofproto/ofproto-dpif-trace.h"
> #include "ofproto/ofproto-dpif-xlate-cache.h"
> @@ -117,6 +118,7 @@ struct xbridge {
> struct dpif_sflow *sflow; /* SFlow handle, or null. */
> struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
> struct netflow *netflow; /* Netflow handle, or null. */
> +struct dpif_lsample *lsample; /* Local sample handle, or null. */
> struct stp *stp; /* STP or null if disabled. */
> struct rstp *rstp;/* RSTP or null if disabled. */
>
> @@ -687,6 +689,7 @@ static void xlate_xbridge_set(struct xbridge *, struct
> dpif *,
>const struct dpif_sflow *,
>const struct dpif_ipfix *,
>const struct netflow *,
> + const struct dpif_lsample *,
>bool forward_bpdu, bool has_in_band,
>const struct dpif_backer_support *,
>const struct xbridge_addr *);
> @@ -1070,6 +1073,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
>const struct dpif_sflow *sflow,
>const struct dpif_ipfix *ipfix,
>const struct netflow *netflow,
> + const struct dpif_lsample *lsample,
nit: I would move above netflow, as you also do the actual init/unref before
netflow.
>bool forward_bpdu, bool has_in_band,
>const struct dpif_backer_support *support,
>const struct xbridge_addr *addr)
> @@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge,
> xbridge->ipfix = dpif_ipfix_ref(ipfix);
> }
>
> +if (xbridge->lsample != lsample) {
> +dpif_lsample_unref(xbridge->lsample);
> +xbridge->lsample = dpif_lsample_ref(lsample);
> +}
> +
> if (xbridge->stp != stp) {
> stp_unref(xbridge->stp);
> xbridge->stp = stp_ref(stp);
> @@ -1214,8 +1223,9 @@ xlate_xbridge_copy(struct xbridge *xbridge)
>xbridge->dpif, xbridge->ml, xbridge->stp,
>xbridge->rstp, xbridge->ms, xbridge->mbridge,
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On Wed, Jun 26, 2024 at 11:28:57AM GMT, Ilya Maximets wrote: > On 6/26/24 07:14, Adrián Moreno wrote: > > On Tue, Jun 25, 2024 at 11:04:42PM GMT, Ilya Maximets wrote: > >> On 6/25/24 22:53, Adrián Moreno wrote: > >>> On Mon, Jun 24, 2024 at 07:43:32PM GMT, Adrián Moreno wrote: > On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote: > > On 6/24/24 15:19, Adrián Moreno wrote: > >> On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: > >>> On 6/5/24 22:23, Adrian Moreno wrote: > Use the newly added emit_sample to implement OpenFlow sample() > actions > with local sampling configuration. > > Signed-off-by: Adrian Moreno > --- > ofproto/ofproto-dpif-lsample.c | 17 > ofproto/ofproto-dpif-lsample.h | 6 ++ > ofproto/ofproto-dpif-xlate.c | 163 > - > ofproto/ofproto-dpif-xlate.h | 3 +- > ofproto/ofproto-dpif.c | 2 +- > 5 files changed, 144 insertions(+), 47 deletions(-) > >>> > >>> Not a full review, just a note that this patch needs tests in > >>> ofproto-dpif.at > >>> that would check that we generate correct datapath flows. > >>> > >> > >> I thought about that, but ofproto-dpif.at is based on dummy datapat > >> (userspace) which does not support this action. The configuration will > >> be ignored and the datapath actions will not be generated. That's why I > >> relied on system-traffic.at tests. Do you see any way around this? > > > > We don't need actual datapath support if we're not sending any actual > > trafic. You should be able to just turn on the capability and check > > that ofproto/trace generates correct actions. > > > > We test kernel tunnels this way, for example. See the macro > > OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. > > > > >>> > >>> Tried it but it doesn't seem to work. I now remember I hit this issue > >>> originally :-). > >>> We cannot enable a capability beyond the datapath's "boot time" > >>> capabilities. > >>> > >>> If you try to run > >>> "ovs-appctl dpif/set-dp-features br0 emit_sample true", the command > >>> fails with: > >>> "Can not enable features not supported by the datapth" > >>> > >>> The safeguard does make sense in general (prevents users from > >>> knee-capping themselves) but for testing maybe we want to force it? > >>> > >>> OTOH, marking this feature as supported in the userspace datapath would > >>> not be catastrophic, we could just warn the action is not supported > >>> in odp_execute(), or consider VLOG_DBG_RL as current implementation > >>> until we come up with a good one. However, I cannot of a good way of > >>> of properly reporting this to the user beforehand so I think it could > >>> still cause a lot of confusion. > >>> > >>> Thoughts? > >> > >> Hmm. This command is generally for testing only or experiments. > >> It's not documented and hence we do not support changing datapath > >> features in runtime. > > > > Oh! It's true that it's docummented, although it does appear in > > "list-commands". I thought we supported disabling features, I guess we > > might on a case-by-case basis and through OVSDB. > > > >> > >> It should be fine to add another command like force-dp-features to > >> overcome the restriction. Just make sure that it is not documented > >> and doesn't appear in the list-commands output. > >> > > > > If "set-dp-features" is already an exceptional path, can't we just add a > > "--force" option to it? > > I guess, it's fine to add a flag. Just don't document it. But, please, > add a scary warning in this case saying that unsupported feature is enabled > and the behavior is unpredictable from this point on. Tests can ignore the > warning. > I will not documment it but I don't think we should remove from "list-commands" either. And sure, I'll add a scary warning. Thanks. Adrián ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On 6/26/24 07:14, Adrián Moreno wrote: > On Tue, Jun 25, 2024 at 11:04:42PM GMT, Ilya Maximets wrote: >> On 6/25/24 22:53, Adrián Moreno wrote: >>> On Mon, Jun 24, 2024 at 07:43:32PM GMT, Adrián Moreno wrote: On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote: > On 6/24/24 15:19, Adrián Moreno wrote: >> On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: >>> On 6/5/24 22:23, Adrian Moreno wrote: Use the newly added emit_sample to implement OpenFlow sample() actions with local sampling configuration. Signed-off-by: Adrian Moreno --- ofproto/ofproto-dpif-lsample.c | 17 ofproto/ofproto-dpif-lsample.h | 6 ++ ofproto/ofproto-dpif-xlate.c | 163 - ofproto/ofproto-dpif-xlate.h | 3 +- ofproto/ofproto-dpif.c | 2 +- 5 files changed, 144 insertions(+), 47 deletions(-) >>> >>> Not a full review, just a note that this patch needs tests in >>> ofproto-dpif.at >>> that would check that we generate correct datapath flows. >>> >> >> I thought about that, but ofproto-dpif.at is based on dummy datapat >> (userspace) which does not support this action. The configuration will >> be ignored and the datapath actions will not be generated. That's why I >> relied on system-traffic.at tests. Do you see any way around this? > > We don't need actual datapath support if we're not sending any actual > trafic. You should be able to just turn on the capability and check > that ofproto/trace generates correct actions. > > We test kernel tunnels this way, for example. See the macro > OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. > >>> >>> Tried it but it doesn't seem to work. I now remember I hit this issue >>> originally :-). >>> We cannot enable a capability beyond the datapath's "boot time" >>> capabilities. >>> >>> If you try to run >>> "ovs-appctl dpif/set-dp-features br0 emit_sample true", the command >>> fails with: >>> "Can not enable features not supported by the datapth" >>> >>> The safeguard does make sense in general (prevents users from >>> knee-capping themselves) but for testing maybe we want to force it? >>> >>> OTOH, marking this feature as supported in the userspace datapath would >>> not be catastrophic, we could just warn the action is not supported >>> in odp_execute(), or consider VLOG_DBG_RL as current implementation >>> until we come up with a good one. However, I cannot of a good way of >>> of properly reporting this to the user beforehand so I think it could >>> still cause a lot of confusion. >>> >>> Thoughts? >> >> Hmm. This command is generally for testing only or experiments. >> It's not documented and hence we do not support changing datapath >> features in runtime. > > Oh! It's true that it's docummented, although it does appear in > "list-commands". I thought we supported disabling features, I guess we > might on a case-by-case basis and through OVSDB. > >> >> It should be fine to add another command like force-dp-features to >> overcome the restriction. Just make sure that it is not documented >> and doesn't appear in the list-commands output. >> > > If "set-dp-features" is already an exceptional path, can't we just add a > "--force" option to it? I guess, it's fine to add a flag. Just don't document it. But, please, add a scary warning in this case saying that unsupported feature is enabled and the behavior is unpredictable from this point on. Tests can ignore the warning. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On Tue, Jun 25, 2024 at 11:04:42PM GMT, Ilya Maximets wrote: > On 6/25/24 22:53, Adrián Moreno wrote: > > On Mon, Jun 24, 2024 at 07:43:32PM GMT, Adrián Moreno wrote: > >> On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote: > >>> On 6/24/24 15:19, Adrián Moreno wrote: > On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: > > On 6/5/24 22:23, Adrian Moreno wrote: > >> Use the newly added emit_sample to implement OpenFlow sample() actions > >> with local sampling configuration. > >> > >> Signed-off-by: Adrian Moreno > >> --- > >> ofproto/ofproto-dpif-lsample.c | 17 > >> ofproto/ofproto-dpif-lsample.h | 6 ++ > >> ofproto/ofproto-dpif-xlate.c | 163 - > >> ofproto/ofproto-dpif-xlate.h | 3 +- > >> ofproto/ofproto-dpif.c | 2 +- > >> 5 files changed, 144 insertions(+), 47 deletions(-) > > > > Not a full review, just a note that this patch needs tests in > > ofproto-dpif.at > > that would check that we generate correct datapath flows. > > > > I thought about that, but ofproto-dpif.at is based on dummy datapat > (userspace) which does not support this action. The configuration will > be ignored and the datapath actions will not be generated. That's why I > relied on system-traffic.at tests. Do you see any way around this? > >>> > >>> We don't need actual datapath support if we're not sending any actual > >>> trafic. You should be able to just turn on the capability and check > >>> that ofproto/trace generates correct actions. > >>> > >>> We test kernel tunnels this way, for example. See the macro > >>> OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. > >>> > >> > > > > Tried it but it doesn't seem to work. I now remember I hit this issue > > originally :-). > > We cannot enable a capability beyond the datapath's "boot time" > > capabilities. > > > > If you try to run > > "ovs-appctl dpif/set-dp-features br0 emit_sample true", the command > > fails with: > > "Can not enable features not supported by the datapth" > > > > The safeguard does make sense in general (prevents users from > > knee-capping themselves) but for testing maybe we want to force it? > > > > OTOH, marking this feature as supported in the userspace datapath would > > not be catastrophic, we could just warn the action is not supported > > in odp_execute(), or consider VLOG_DBG_RL as current implementation > > until we come up with a good one. However, I cannot of a good way of > > of properly reporting this to the user beforehand so I think it could > > still cause a lot of confusion. > > > > Thoughts? > > Hmm. This command is generally for testing only or experiments. > It's not documented and hence we do not support changing datapath > features in runtime. Oh! It's true that it's docummented, although it does appear in "list-commands". I thought we supported disabling features, I guess we might on a case-by-case basis and through OVSDB. > > It should be fine to add another command like force-dp-features to > overcome the restriction. Just make sure that it is not documented > and doesn't appear in the list-commands output. > If "set-dp-features" is already an exceptional path, can't we just add a "--force" option to it? Thanks. Adrián ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On 6/25/24 22:53, Adrián Moreno wrote: > On Mon, Jun 24, 2024 at 07:43:32PM GMT, Adrián Moreno wrote: >> On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote: >>> On 6/24/24 15:19, Adrián Moreno wrote: On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: > On 6/5/24 22:23, Adrian Moreno wrote: >> Use the newly added emit_sample to implement OpenFlow sample() actions >> with local sampling configuration. >> >> Signed-off-by: Adrian Moreno >> --- >> ofproto/ofproto-dpif-lsample.c | 17 >> ofproto/ofproto-dpif-lsample.h | 6 ++ >> ofproto/ofproto-dpif-xlate.c | 163 - >> ofproto/ofproto-dpif-xlate.h | 3 +- >> ofproto/ofproto-dpif.c | 2 +- >> 5 files changed, 144 insertions(+), 47 deletions(-) > > Not a full review, just a note that this patch needs tests in > ofproto-dpif.at > that would check that we generate correct datapath flows. > I thought about that, but ofproto-dpif.at is based on dummy datapat (userspace) which does not support this action. The configuration will be ignored and the datapath actions will not be generated. That's why I relied on system-traffic.at tests. Do you see any way around this? >>> >>> We don't need actual datapath support if we're not sending any actual >>> trafic. You should be able to just turn on the capability and check >>> that ofproto/trace generates correct actions. >>> >>> We test kernel tunnels this way, for example. See the macro >>> OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. >>> >> > > Tried it but it doesn't seem to work. I now remember I hit this issue > originally :-). > We cannot enable a capability beyond the datapath's "boot time" > capabilities. > > If you try to run > "ovs-appctl dpif/set-dp-features br0 emit_sample true", the command > fails with: > "Can not enable features not supported by the datapth" > > The safeguard does make sense in general (prevents users from > knee-capping themselves) but for testing maybe we want to force it? > > OTOH, marking this feature as supported in the userspace datapath would > not be catastrophic, we could just warn the action is not supported > in odp_execute(), or consider VLOG_DBG_RL as current implementation > until we come up with a good one. However, I cannot of a good way of > of properly reporting this to the user beforehand so I think it could > still cause a lot of confusion. > > Thoughts? Hmm. This command is generally for testing only or experiments. It's not documented and hence we do not support changing datapath features in runtime. It should be fine to add another command like force-dp-features to overcome the restriction. Just make sure that it is not documented and doesn't appear in the list-commands output. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On Mon, Jun 24, 2024 at 07:43:32PM GMT, Adrián Moreno wrote: > On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote: > > On 6/24/24 15:19, Adrián Moreno wrote: > > > On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: > > >> On 6/5/24 22:23, Adrian Moreno wrote: > > >>> Use the newly added emit_sample to implement OpenFlow sample() actions > > >>> with local sampling configuration. > > >>> > > >>> Signed-off-by: Adrian Moreno > > >>> --- > > >>> ofproto/ofproto-dpif-lsample.c | 17 > > >>> ofproto/ofproto-dpif-lsample.h | 6 ++ > > >>> ofproto/ofproto-dpif-xlate.c | 163 - > > >>> ofproto/ofproto-dpif-xlate.h | 3 +- > > >>> ofproto/ofproto-dpif.c | 2 +- > > >>> 5 files changed, 144 insertions(+), 47 deletions(-) > > >> > > >> Not a full review, just a note that this patch needs tests in > > >> ofproto-dpif.at > > >> that would check that we generate correct datapath flows. > > >> > > > > > > I thought about that, but ofproto-dpif.at is based on dummy datapat > > > (userspace) which does not support this action. The configuration will > > > be ignored and the datapath actions will not be generated. That's why I > > > relied on system-traffic.at tests. Do you see any way around this? > > > > We don't need actual datapath support if we're not sending any actual > > trafic. You should be able to just turn on the capability and check > > that ofproto/trace generates correct actions. > > > > We test kernel tunnels this way, for example. See the macro > > OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. > > > Tried it but it doesn't seem to work. I now remember I hit this issue originally :-). We cannot enable a capability beyond the datapath's "boot time" capabilities. If you try to run "ovs-appctl dpif/set-dp-features br0 emit_sample true", the command fails with: "Can not enable features not supported by the datapth" The safeguard does make sense in general (prevents users from knee-capping themselves) but for testing maybe we want to force it? OTOH, marking this feature as supported in the userspace datapath would not be catastrophic, we could just warn the action is not supported in odp_execute(), or consider VLOG_DBG_RL as current implementation until we come up with a good one. However, I cannot of a good way of of properly reporting this to the user beforehand so I think it could still cause a lot of confusion. Thoughts? Thanks. Adrián ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote: > On 6/24/24 15:19, Adrián Moreno wrote: > > On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: > >> On 6/5/24 22:23, Adrian Moreno wrote: > >>> Use the newly added emit_sample to implement OpenFlow sample() actions > >>> with local sampling configuration. > >>> > >>> Signed-off-by: Adrian Moreno > >>> --- > >>> ofproto/ofproto-dpif-lsample.c | 17 > >>> ofproto/ofproto-dpif-lsample.h | 6 ++ > >>> ofproto/ofproto-dpif-xlate.c | 163 - > >>> ofproto/ofproto-dpif-xlate.h | 3 +- > >>> ofproto/ofproto-dpif.c | 2 +- > >>> 5 files changed, 144 insertions(+), 47 deletions(-) > >> > >> Not a full review, just a note that this patch needs tests in > >> ofproto-dpif.at > >> that would check that we generate correct datapath flows. > >> > > > > I thought about that, but ofproto-dpif.at is based on dummy datapat > > (userspace) which does not support this action. The configuration will > > be ignored and the datapath actions will not be generated. That's why I > > relied on system-traffic.at tests. Do you see any way around this? > > We don't need actual datapath support if we're not sending any actual > trafic. You should be able to just turn on the capability and check > that ofproto/trace generates correct actions. > > We test kernel tunnels this way, for example. See the macro > OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. > OK. Thanks for the pointer. I'll give it a try. Adrián. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On 6/24/24 15:19, Adrián Moreno wrote: > On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: >> On 6/5/24 22:23, Adrian Moreno wrote: >>> Use the newly added emit_sample to implement OpenFlow sample() actions >>> with local sampling configuration. >>> >>> Signed-off-by: Adrian Moreno >>> --- >>> ofproto/ofproto-dpif-lsample.c | 17 >>> ofproto/ofproto-dpif-lsample.h | 6 ++ >>> ofproto/ofproto-dpif-xlate.c | 163 - >>> ofproto/ofproto-dpif-xlate.h | 3 +- >>> ofproto/ofproto-dpif.c | 2 +- >>> 5 files changed, 144 insertions(+), 47 deletions(-) >> >> Not a full review, just a note that this patch needs tests in ofproto-dpif.at >> that would check that we generate correct datapath flows. >> > > I thought about that, but ofproto-dpif.at is based on dummy datapat > (userspace) which does not support this action. The configuration will > be ignored and the datapath actions will not be generated. That's why I > relied on system-traffic.at tests. Do you see any way around this? We don't need actual datapath support if we're not sending any actual trafic. You should be able to just turn on the capability and check that ofproto/trace generates correct actions. We test kernel tunnels this way, for example. See the macro OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: > On 6/5/24 22:23, Adrian Moreno wrote: > > Use the newly added emit_sample to implement OpenFlow sample() actions > > with local sampling configuration. > > > > Signed-off-by: Adrian Moreno > > --- > > ofproto/ofproto-dpif-lsample.c | 17 > > ofproto/ofproto-dpif-lsample.h | 6 ++ > > ofproto/ofproto-dpif-xlate.c | 163 - > > ofproto/ofproto-dpif-xlate.h | 3 +- > > ofproto/ofproto-dpif.c | 2 +- > > 5 files changed, 144 insertions(+), 47 deletions(-) > > Not a full review, just a note that this patch needs tests in ofproto-dpif.at > that would check that we generate correct datapath flows. > I thought about that, but ofproto-dpif.at is based on dummy datapat (userspace) which does not support this action. The configuration will be ignored and the datapath actions will not be generated. That's why I relied on system-traffic.at tests. Do you see any way around this? Thanks. Adrián ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On 6/5/24 22:23, Adrian Moreno wrote: > Use the newly added emit_sample to implement OpenFlow sample() actions > with local sampling configuration. > > Signed-off-by: Adrian Moreno > --- > ofproto/ofproto-dpif-lsample.c | 17 > ofproto/ofproto-dpif-lsample.h | 6 ++ > ofproto/ofproto-dpif-xlate.c | 163 - > ofproto/ofproto-dpif-xlate.h | 3 +- > ofproto/ofproto-dpif.c | 2 +- > 5 files changed, 144 insertions(+), 47 deletions(-) Not a full review, just a note that this patch needs tests in ofproto-dpif.at that would check that we generate correct datapath flows. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
Bleep bloop. Greetings Adrian Moreno, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
Use the newly added emit_sample to implement OpenFlow sample() actions
with local sampling configuration.
Signed-off-by: Adrian Moreno
---
ofproto/ofproto-dpif-lsample.c | 17
ofproto/ofproto-dpif-lsample.h | 6 ++
ofproto/ofproto-dpif-xlate.c | 163 -
ofproto/ofproto-dpif-xlate.h | 3 +-
ofproto/ofproto-dpif.c | 2 +-
5 files changed, 144 insertions(+), 47 deletions(-)
diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
index 7bdafac19..2c0b5da89 100644
--- a/ofproto/ofproto-dpif-lsample.c
+++ b/ofproto/ofproto-dpif-lsample.c
@@ -139,6 +139,23 @@ dpif_lsample_set_options(struct dpif_lsample *lsample,
return changed;
}
+/* Returns the group_id of the exporter with the given collector_set_id, if it
+ * exists. */
+bool
+dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t collector_set_id,
+ uint32_t *group_id)
+{
+struct lsample_exporter_node *node;
+bool found = false;
+
+node = dpif_lsample_find_exporter_node(ps, collector_set_id);
+if (node) {
+found = true;
+*group_id = node->exporter.options.group_id;
+}
+return found;
+}
+
struct dpif_lsample *
dpif_lsample_create(void)
{
diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
index c23ea8372..f13425575 100644
--- a/ofproto/ofproto-dpif-lsample.h
+++ b/ofproto/ofproto-dpif-lsample.h
@@ -19,6 +19,7 @@
#include
#include
+#include
struct dpif_lsample;
struct ofproto_lsample_options;
@@ -31,4 +32,9 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
const struct ofproto_lsample_options *,
size_t n_opts);
+bool dpif_lsample_get_group_id(struct dpif_lsample *,
+ uint32_t collector_set_id,
+ uint32_t *group_id);
+
#endif /* OFPROTO_DPIF_LSAMPLE_H */
+
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7c4950895..5bd215d31 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -47,6 +47,7 @@
#include "ofproto/ofproto-dpif-ipfix.h"
#include "ofproto/ofproto-dpif-mirror.h"
#include "ofproto/ofproto-dpif-monitor.h"
+#include "ofproto/ofproto-dpif-lsample.h"
#include "ofproto/ofproto-dpif-sflow.h"
#include "ofproto/ofproto-dpif-trace.h"
#include "ofproto/ofproto-dpif-xlate-cache.h"
@@ -117,6 +118,7 @@ struct xbridge {
struct dpif_sflow *sflow; /* SFlow handle, or null. */
struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
struct netflow *netflow; /* Netflow handle, or null. */
+struct dpif_lsample *lsample; /* Local sample handle, or null. */
struct stp *stp; /* STP or null if disabled. */
struct rstp *rstp;/* RSTP or null if disabled. */
@@ -687,6 +689,7 @@ static void xlate_xbridge_set(struct xbridge *, struct dpif
*,
const struct dpif_sflow *,
const struct dpif_ipfix *,
const struct netflow *,
+ const struct dpif_lsample *,
bool forward_bpdu, bool has_in_band,
const struct dpif_backer_support *,
const struct xbridge_addr *);
@@ -1070,6 +1073,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
const struct dpif_sflow *sflow,
const struct dpif_ipfix *ipfix,
const struct netflow *netflow,
+ const struct dpif_lsample *lsample,
bool forward_bpdu, bool has_in_band,
const struct dpif_backer_support *support,
const struct xbridge_addr *addr)
@@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge,
xbridge->ipfix = dpif_ipfix_ref(ipfix);
}
+if (xbridge->lsample != lsample) {
+dpif_lsample_unref(xbridge->lsample);
+xbridge->lsample = dpif_lsample_ref(lsample);
+}
+
if (xbridge->stp != stp) {
stp_unref(xbridge->stp);
xbridge->stp = stp_ref(stp);
@@ -1214,8 +1223,9 @@ xlate_xbridge_copy(struct xbridge *xbridge)
xbridge->dpif, xbridge->ml, xbridge->stp,
xbridge->rstp, xbridge->ms, xbridge->mbridge,
xbridge->sflow, xbridge->ipfix, xbridge->netflow,
- xbridge->forward_bpdu, xbridge->has_in_band,
- >support, xbridge->addr);
+ xbridge->lsample, xbridge->forward_bpdu,
+ xbridge->has_in_band, >support,
+ xbridge->addr);
LIST_FOR_EACH (xbundle, list_node, >xbundles) {
xlate_xbundle_copy(new_xbridge, xbundle);
}
@@ -1373,6 +1383,7 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, const
char *name,
const struct
