On 27 Jun 2024, at 13:16, Adrián Moreno wrote:
> 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 <[email protected]> >> >> 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. This will be hard to debug, so I would prefer doing it through dpif_lsample_delete_exporter() >>> + } >>> + 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. ACK, was not aware that this was already done as part of the API. >> 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. ACK, I tried to find a way to abuse this but was not successful so I’m fine with leaving it as is. >>> + 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
