On Tue, Jul 09, 2024 at 11:45:19AM GMT, Eelco Chaudron wrote:
> On 7 Jul 2024, at 22:08, Adrian Moreno wrote:
>
> > Add a new resource in ofproto-dpif and the corresponding API in
> > ofproto_provider.h to represent and local sampling configuration.
> >
> > Signed-off-by: Adrian Moreno <amore...@redhat.com>
>
> Some small nits below.
>
> //Eelco
>
> > ---
> > ofproto/automake.mk | 2 +
> > ofproto/ofproto-dpif-lsample.c | 187 +++++++++++++++++++++++++++++++++
> > ofproto/ofproto-dpif-lsample.h | 36 +++++++
> > ofproto/ofproto-dpif.c | 38 +++++++
> > ofproto/ofproto-dpif.h | 1 +
> > ofproto/ofproto-provider.h | 9 ++
> > ofproto/ofproto.c | 12 +++
> > ofproto/ofproto.h | 8 ++
> > 8 files changed, 293 insertions(+)
> > create mode 100644 ofproto/ofproto-dpif-lsample.c
> > create mode 100644 ofproto/ofproto-dpif-lsample.h
> >
> > diff --git a/ofproto/automake.mk b/ofproto/automake.mk
> > index 7c08b563b..cb1361b8a 100644
> > --- a/ofproto/automake.mk
> > +++ b/ofproto/automake.mk
> > @@ -30,6 +30,8 @@ ofproto_libofproto_la_SOURCES = \
> > ofproto/ofproto-dpif.h \
> > ofproto/ofproto-dpif-ipfix.c \
> > ofproto/ofproto-dpif-ipfix.h \
> > + ofproto/ofproto-dpif-lsample.c \
> > + ofproto/ofproto-dpif-lsample.h \
> > ofproto/ofproto-dpif-mirror.c \
> > ofproto/ofproto-dpif-mirror.h \
> > ofproto/ofproto-dpif-monitor.c \
> > diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> > new file mode 100644
> > index 000000000..d675a116f
> > --- /dev/null
> > +++ b/ofproto/ofproto-dpif-lsample.c
> > @@ -0,0 +1,187 @@
> > +/*
> > + * Copyright (c) 2024 Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +#include "ofproto-dpif-lsample.h"
> > +
> > +#include "cmap.h"
> > +#include "hash.h"
> > +#include "ofproto.h"
> > +#include "openvswitch/thread.h"
> > +
> > +/* Dpif local sampling.
> > + *
> > + * Thread safety: dpif_lsample allows lockless concurrent reads of local
> > + * sampling exporters as long as the following restrictions are met:
> > + * 1) While the last reference is being dropped, i.e: a thread is calling
> > + * "dpif_lsample_unref" on the last reference, other threads cannot
> > call
> > + * "dpif_lsample_ref".
> > + * 2) Threads do not quiese while holding references to internal
> > + * lsample_exporter objects.
> > + */
> > +
> > +struct dpif_lsample {
> > + struct cmap exporters; /* Contains lsample_exporter_node
> > instances
> > + indexed by collector_set_id. */
> > + struct ovs_mutex mutex; /* Protects concurrent
> > insertion/deletion
> > + * of exporters. */
> > +
> > + struct ovs_refcount ref_cnt; /* Controls references to this
> > instance. */
> > +};
> > +
> > +struct lsample_exporter {
> > + struct ofproto_lsample_options options;
> > +};
> > +
> > +struct lsample_exporter_node {
> > + struct cmap_node node; /* In dpif_lsample->exporters. */
> > + struct lsample_exporter exporter;
> > +};
> > +
> > +static void
> > +dpif_lsample_delete_exporter(struct dpif_lsample *lsample,
> > + struct lsample_exporter_node *node)
> > +{
> > + ovs_mutex_lock(&lsample->mutex);
> > + cmap_remove(&lsample->exporters, &node->node,
> > + hash_int(node->exporter.options.collector_set_id, 0));
> > + ovs_mutex_unlock(&lsample->mutex);
> > +
> > + ovsrcu_postpone(free, node);
> > +}
> > +
> > +/* Adds an exporter with the provided options which are copied. */
> > +static struct lsample_exporter_node *
> > +dpif_lsample_add_exporter(struct dpif_lsample *lsample,
> > + const struct ofproto_lsample_options *options)
> > +{
> > + struct lsample_exporter_node *node;
> > +
> > + node = xzalloc(sizeof *node);
> > + node->exporter.options = *options;
> > +
> > + ovs_mutex_lock(&lsample->mutex);
> > + cmap_insert(&lsample->exporters, &node->node,
> > + hash_int(options->collector_set_id, 0));
> > + ovs_mutex_unlock(&lsample->mutex);
> > +
> > + return node;
> > +}
> > +
> > +static struct lsample_exporter_node *
> > +dpif_lsample_find_exporter_node(const struct dpif_lsample *lsample,
> > + const uint32_t collector_set_id)
> > +{
> > + struct lsample_exporter_node *node;
> > +
> > + CMAP_FOR_EACH_WITH_HASH (node, node,
> > + hash_int(collector_set_id, 0),
> > + &lsample->exporters) {
> > + if (node->exporter.options.collector_set_id == collector_set_id) {
> > + return node;
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > +/* Sets the lsample configuration and returns true if the configuration
> > + * has changed. */
> > +bool
> > +dpif_lsample_set_options(struct dpif_lsample *lsample,
> > + const struct ofproto_lsample_options *options,
> > + size_t n_options)
> > +{
> > + const struct ofproto_lsample_options *opt;
> > + struct lsample_exporter_node *node;
> > + bool changed = false;
> > + int i;
> > +
> > + for (i = 0; i < n_options; i++) {
> > + opt = &options[i];
> > + node = dpif_lsample_find_exporter_node(lsample,
> > + opt->collector_set_id);
> > + if (!node) {
> > + dpif_lsample_add_exporter(lsample, opt);
> > + changed = true;
> > + } else if (memcmp(&node->exporter.options, opt, sizeof(*opt))) {
> > + dpif_lsample_delete_exporter(lsample, node);
> > + dpif_lsample_add_exporter(lsample, opt);
> > + changed = true;
> > + }
> > + }
> > +
> > + /* Delete exporters that have been removed. */
> > + CMAP_FOR_EACH (node, node, &lsample->exporters) {
> > + for (i = 0; i < n_options; i++) {
> > + if (node->exporter.options.collector_set_id
> > + == options[i].collector_set_id) {
> > + break;
> > + }
> > + }
> > + if (i == n_options) {
> > + dpif_lsample_delete_exporter(lsample, node);
> > + changed = true;
> > + }
> > + }
> > +
> > + return changed;
> > +}
> > +
> > +struct dpif_lsample *
> > +dpif_lsample_create(void)
> > +{
> > + struct dpif_lsample *lsample;
> > +
> > + lsample = xzalloc(sizeof *lsample);
> > + cmap_init(&lsample->exporters);
> > + ovs_mutex_init(&lsample->mutex);
> > + ovs_refcount_init(&lsample->ref_cnt);
> > +
> > + return lsample;
> > +}
> > +
> > +static void
> > +dpif_lsample_destroy(struct dpif_lsample *lsample)
> > +{
> > + if (lsample) {
> > + struct lsample_exporter_node *node;
> > +
> > + CMAP_FOR_EACH (node, node, &lsample->exporters) {
> > + dpif_lsample_delete_exporter(lsample, node);
> > + }
> > + cmap_destroy(&lsample->exporters);
> > + free(lsample);
> > + }
> > +}
> > +
> > +struct dpif_lsample *
> > +dpif_lsample_ref(const struct dpif_lsample *lsample_)
> > +{
> > + struct dpif_lsample *lsample = CONST_CAST(struct dpif_lsample *,
> > lsample_);
> > +
> > + if (lsample) {
> > + ovs_refcount_ref(&lsample->ref_cnt);
> > + }
> > + return lsample;
> > +}
> > +
> > +void
> > +dpif_lsample_unref(struct dpif_lsample *lsample)
> > +{
> > + if (lsample && ovs_refcount_unref_relaxed(&lsample->ref_cnt) == 1) {
> > + dpif_lsample_destroy(lsample);
> > + }
> > +}
> > diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
> > new file mode 100644
> > index 000000000..bee36c9c5
> > --- /dev/null
> > +++ b/ofproto/ofproto-dpif-lsample.h
> > @@ -0,0 +1,36 @@
> > +/*
> > + * Copyright (c) 2024 Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef OFPROTO_DPIF_LSAMPLE_H
> > +#define OFPROTO_DPIF_LSAMPLE_H 1
> > +
> > +#include <stdbool.h>
> > +#include <stdlib.h>
> > +
> > +struct dpif_lsample;
> > +struct ofproto_lsample_options;
> > +
> > +struct dpif_lsample *dpif_lsample_create(void);
> > +
> > +void dpif_lsample_unref(struct dpif_lsample *);
> > +
> > +struct dpif_lsample *dpif_lsample_ref(const struct dpif_lsample *);
>
> I would define ref before unref.
>
> > +
> > +bool dpif_lsample_set_options(struct dpif_lsample *,
> > + const struct ofproto_lsample_options *,
> > + size_t n_opts);
>
> Also not sure what would be the right way to group, not group, add extra
> new lines in include files. Looks like we sort of group similar function,
> and add new lines between groups. If we follow that it should probably look
> something like this:
>
> struct dpif_lsample *dpif_lsample_create(void);
>
> struct dpif_lsample *dpif_lsample_ref(const struct dpif_lsample *);
> void dpif_lsample_unref(struct dpif_lsample *);
>
> bool dpif_lsample_set_options(struct dpif_lsample *,
> const struct ofproto_lsample_options *,
> size_t n_opts);
>
> But I have no real preferences, maybe Ilya has.
>
I don't either, your sorting looks good.
> > +#endif /* OFPROTO_DPIF_LSAMPLE_H */
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 94c84d697..56830c630 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -49,6 +49,7 @@
> > #include "ofproto-dpif-sflow.h"
> > #include "ofproto-dpif-trace.h"
> > #include "ofproto-dpif-upcall.h"
> > +#include "ofproto-dpif-lsample.h"
> > #include "ofproto-dpif-xlate.h"
> > #include "ofproto-dpif-xlate-cache.h"
> > #include "openvswitch/ofp-actions.h"
> > @@ -1954,6 +1955,7 @@ destruct(struct ofproto *ofproto_, bool del)
> > netflow_unref(ofproto->netflow);
> > dpif_sflow_unref(ofproto->sflow);
> > dpif_ipfix_unref(ofproto->ipfix);
> > + dpif_lsample_unref(ofproto->lsample);
> > hmap_destroy(&ofproto->bundles);
> > mac_learning_unref(ofproto->ml);
> > mcast_snooping_unref(ofproto->ms);
> > @@ -2513,6 +2515,41 @@ get_ipfix_stats(const struct ofproto *ofproto_,
> > return dpif_ipfix_get_stats(di, bridge_ipfix, replies);
> > }
> >
> > +static int
> > +set_local_sample(struct ofproto *ofproto_,
> > + const struct ofproto_lsample_options *options,
> > + size_t n_opts)
> > +{
> > + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> > + struct dpif_lsample *lsample = ofproto->lsample;
> > + bool changed = false;
> > +
> > + if (!ofproto->backer->rt_support.psample) {
> > + return ENOTSUP;
> > + }
> > +
> > + if (n_opts && !lsample) {
> > + lsample = ofproto->lsample = dpif_lsample_create();
> > + changed = true;
> > + }
> > +
> > + if (lsample) {
> > + if (!n_opts) {
> > + dpif_lsample_unref(lsample);
> > + lsample = ofproto->lsample = NULL;
> > + changed = true;
> > + } else {
> > + changed |= dpif_lsample_set_options(lsample, options, n_opts);
> > + }
> > + }
> > +
> > + if (changed) {
> > + ofproto->backer->need_revalidate = REV_RECONFIGURE;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int
> > set_cfm(struct ofport *ofport_, const struct cfm_settings *s)
> > {
> > @@ -7100,6 +7137,7 @@ const struct ofproto_class ofproto_dpif_class = {
> > get_netflow_ids,
> > set_sflow,
> > set_ipfix,
> > + set_local_sample,
> > get_ipfix_stats,
> > set_cfm,
> > cfm_status_changed,
> > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> > index bc7a19dab..420c350fd 100644
> > --- a/ofproto/ofproto-dpif.h
> > +++ b/ofproto/ofproto-dpif.h
> > @@ -331,6 +331,7 @@ struct ofproto_dpif {
> > struct netflow *netflow;
> > struct dpif_sflow *sflow;
> > struct dpif_ipfix *ipfix;
> > + struct dpif_lsample *lsample;
> > struct hmap bundles; /* Contains "struct ofbundle"s. */
> > struct mac_learning *ml;
> > struct mcast_snooping *ms;
> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index 83c509fcf..02e6710d9 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -1479,6 +1479,15 @@ struct ofproto_class {
> > const struct ofproto_ipfix_flow_exporter_options
> > *flow_exporters_options, size_t n_flow_exporters_options);
> >
> > + /* Configures local sampling on 'ofproto' according to the options
> > array
> > + * of 'options' which contains 'n_options' elements.
> > + *
> > + * EOPNOTSUPP as a return value indicates that 'ofproto' does not
> > support
> > + * local sampling. */
> > + int (*set_local_sample)(struct ofproto *ofproto,
> > + const struct ofproto_lsample_options *options,
> > + size_t n_options);
> > +
> > /* Gets IPFIX stats on 'ofproto' according to the exporter of birdge
> > * IPFIX or flow-based IPFIX.
> > *
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 21c6a1d82..2a1db8a0a 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -1000,6 +1000,18 @@ ofproto_get_datapath_cap(const char *datapath_type,
> > struct smap *dp_cap)
> > }
> > }
> >
> > +int ofproto_set_local_sample(struct ofproto *ofproto,
> > + const struct ofproto_lsample_options *options,
> > + size_t n_options)
> > +{
> > + if (ofproto->ofproto_class->set_local_sample) {
> > + return ofproto->ofproto_class->set_local_sample(ofproto, options,
> > + n_options);
> > + } else {
> > + return ENOTSUP;
> > + }
> > +}
> > +
> > /* Connection tracking configuration. */
> > void
> > ofproto_ct_set_zone_timeout_policy(const char *datapath_type, uint16_t
> > zone_id,
> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > index 1c07df275..fded3a3db 100644
> > --- a/ofproto/ofproto.h
> > +++ b/ofproto/ofproto.h
> > @@ -103,6 +103,11 @@ struct ofproto_ipfix_flow_exporter_options {
> > char *virtual_obs_id;
> > };
> >
> > +struct ofproto_lsample_options {
> > + uint32_t collector_set_id;
> > + uint32_t group_id;
> > +};
> > +
> > struct ofproto_rstp_status {
> > bool enabled; /* If false, ignore other members. */
> > rstp_identifier root_id;
> > @@ -390,6 +395,9 @@ void ofproto_ct_zone_limit_protection_update(const char
> > *datapath_type,
> > bool protected);
> > void ofproto_get_datapath_cap(const char *datapath_type,
> > struct smap *dp_cap);
> > +int ofproto_set_local_sample(struct ofproto *ofproto,
> > + const struct ofproto_lsample_options *,
> > + size_t n_options);
>
> nit: Maybe move below ofproto_set_ipfix() so all sampling is grouped together.
>
Ack.
> >
> > /* Configuration of ports. */
> > void ofproto_port_unregister(struct ofproto *, ofp_port_t ofp_port);
> > --
> > 2.45.2
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
