On Wed, Jun 26, 2024 at 02:10:27PM GMT, Eelco Chaudron wrote: > On 5 Jun 2024, at 22:23, 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> > > See some comments below. > > Cheers, > > Eelco > > > --- > > ofproto/automake.mk | 2 + > > ofproto/ofproto-dpif-lsample.c | 185 +++++++++++++++++++++++++++++++++ > > ofproto/ofproto-dpif-lsample.h | 34 ++++++ > > ofproto/ofproto-dpif.c | 37 +++++++ > > ofproto/ofproto-dpif.h | 1 + > > ofproto/ofproto-provider.h | 9 ++ > > ofproto/ofproto.c | 12 +++ > > ofproto/ofproto.h | 8 ++ > > 8 files changed, 288 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..fd39bf561 100644 > > --- a/ofproto/automake.mk > > +++ b/ofproto/automake.mk > > @@ -34,6 +34,8 @@ ofproto_libofproto_la_SOURCES = \ > > ofproto/ofproto-dpif-mirror.h \ > > ofproto/ofproto-dpif-monitor.c \ > > ofproto/ofproto-dpif-monitor.h \ > > + ofproto/ofproto-dpif-lsample.c \ > > + ofproto/ofproto-dpif-lsample.h \ > > Guess these need to move before the dpif-m* files. >
Ack > > ofproto/ofproto-dpif-rid.c \ > > ofproto/ofproto-dpif-rid.h \ > > ofproto/ofproto-dpif-sflow.c \ > > diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c > > new file mode 100644 > > index 000000000..7bdafac19 > > --- /dev/null > > +++ b/ofproto/ofproto-dpif-lsample.c > > @@ -0,0 +1,185 @@ > > +/* > > + * 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; > > New line between definitions and code. > Ack. > > + 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) > > +{ > > + struct lsample_exporter_node *node; > > + const struct ofproto_lsample_options *opt; > > Reverse Christmas tree order. > Ack. > > + 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) { > > + free(node); > > Is this always safe? Should we not just call dpif_lsample_delete_exporter() > to be sure? > I think it is. dpif_lsample_destroy is only called when no more references exist and this happens after it has been replaced with the new one and a quiese period has passed (in xlate_txn_commit). I guess it wouldn't hurt a lot if we wait another quiese period to destroy the elements given this is not explicitly stated in the thread-safety comment at the top of the doc. > > + } > > + 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) { > > Should we add some protection against getting a refcount when not > holding a refcount when calling? i.e. &lsample->ref_cnt should be >= 1? > ovs_refcount_ref is: static inline void ovs_refcount_ref(struct ovs_refcount *refcount) { unsigned int old_refcount; atomic_add_explicit(&refcount->count, 1u, &old_refcount, memory_order_relaxed); ovs_assert(old_refcount > 0); } So I think this enough protection. > Maybe the dpif_lsample can be made an RCU pointer, and you can use > ovs_refcount_try_ref_rcu(). Which in general looks more safe, than just > assume people with will do the right thing when cleaning up the pointer. > I guess there are other ways of implementing this, specially if you try to think in a structure that can be used in any possible way. But this structure is meant to be used by xlate layer to lookup collectors and report statistics. It is not the only one. See all the members "struct xbridge". They all have the same thread safety requirements and none are rcu pointers. Why? Because a mechanism was created to handle all structs in the same: xlate_txn_commit and xlate_txn_commit. This mechanism essentially implements an rcu swap of all the members. Maybe we can make the relationship between this struct and xbridge more explicit but I don't think it makes sense to reinvent what is already working for other 9 structures. > > + 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..c23ea8372 > > --- /dev/null > > +++ b/ofproto/ofproto-dpif-lsample.h > > @@ -0,0 +1,34 @@ > > +/* > > + * 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 *); > > + > > +bool dpif_lsample_set_options(struct dpif_lsample *, > > + const struct ofproto_lsample_options *, > > + size_t n_opts); > > + > > +#endif /* OFPROTO_DPIF_LSAMPLE_H */ > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 035479285..067f60df3 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" > > @@ -2515,6 +2516,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.emit_sample) { > > + 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) > > { > > @@ -7085,6 +7121,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 ae6568463..55a15b2a3 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); > > > > /* Configuration of ports. */ > > void ofproto_port_unregister(struct ofproto *, ofp_port_t ofp_port); > > -- > > 2.45.1 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
