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. > +#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. > > /* 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
