On 24 Apr 2024, at 21:53, Adrian Moreno wrote: > Add a new resource in ofproto-dpif and the corresponding API in > ofproto_provider.h to represent and change psample configuration.
See comments below. //Eelco > Signed-off-by: Adrian Moreno <[email protected]> > --- > ofproto/automake.mk | 2 + > ofproto/ofproto-dpif-psample.c | 167 +++++++++++++++++++++++++++++++++ > ofproto/ofproto-dpif-psample.h | 31 ++++++ > ofproto/ofproto-dpif.c | 33 +++++++ > ofproto/ofproto-dpif.h | 1 + > ofproto/ofproto-provider.h | 9 ++ > ofproto/ofproto.c | 10 ++ > ofproto/ofproto.h | 8 ++ > 8 files changed, 261 insertions(+) > create mode 100644 ofproto/ofproto-dpif-psample.c > create mode 100644 ofproto/ofproto-dpif-psample.h > > diff --git a/ofproto/automake.mk b/ofproto/automake.mk > index 7c08b563b..340003e12 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-psample.c \ > + ofproto/ofproto-dpif-psample.h \ > ofproto/ofproto-dpif-rid.c \ > ofproto/ofproto-dpif-rid.h \ > ofproto/ofproto-dpif-sflow.c \ > diff --git a/ofproto/ofproto-dpif-psample.c b/ofproto/ofproto-dpif-psample.c Like all previous patches (and I guess patches to come), we need a better name than psample :) > new file mode 100644 > index 000000000..a83530ed8 > --- /dev/null > +++ b/ofproto/ofproto-dpif-psample.c > @@ -0,0 +1,167 @@ > +/* > + * 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-psample.h" > + > +#include "hash.h" > +#include "ofproto.h" > +#include "openvswitch/hmap.h" > +#include "openvswitch/thread.h" > +#include "openvswitch/vlog.h" > + > +VLOG_DEFINE_THIS_MODULE(psample); There is no logging in this file, so we might as well remove this. > + > +static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER; Can we give this mutex a meaningful name? > + > +struct psample_exporter { > + uint32_t group_id; > + uint32_t collector_set_id; > +}; > + > +struct psample_exporter_map_node { I would call this just ???_exporter_node? > + struct hmap_node node; > + struct psample_exporter exporter; > +}; > + > +struct dpif_psample { > + struct hmap exporters_map; /* Contains psample_exporter_map_node. */ Should we really use a hmap with all this locking? Would a cmap work better, as multiple threads might be calling into xlate_sample_action, and we could create lock contention during upcall/revalidator handling? In addition, using a cmap, might stop us from having to use refcounting, although updating the cmap might need more work than just swapping it out. > + > + struct ovs_refcount ref_cnt; > +}; > + > +/* Exporter handling */ > +static void > +dpif_psample_clear(struct dpif_psample *ps) OVS_REQUIRES(mutex) The _clear() name is a bit confusing, maybe _free/delete_exporters() Also, the ps variable name is confusing to my brain, but hopefully, this will change when removing psample ;) > +{ > + struct psample_exporter_map_node *node; > + > + HMAP_FOR_EACH_POP (node, node, &ps->exporters_map) { > + free(node); > + } > +} > + > +static struct psample_exporter_map_node* > +dpif_psample_new_exporter_node(struct dpif_psample *ps, Maybe _add_ instead of new? With a comment that we copy the options structure? > + const struct ofproto_psample_options *options) > + OVS_REQUIRES(mutex) > +{ > + struct psample_exporter_map_node *node; > + node = xzalloc(sizeof *node); > + node->exporter.collector_set_id = options->collector_set_id; > + node->exporter.group_id = options->group_id; Rather than setting each option individually, would it be more future prove to just copy the entire structure? It would avoid the zmalloc. > + hmap_insert(&ps->exporters_map, &node->node, > + hash_int(options->collector_set_id, 0)); > + return node; > +} > + > +static struct psample_exporter_map_node* > +dpif_psample_find_exporter_node(const struct dpif_psample *ps, > + const uint32_t collector_set_id) > + OVS_REQUIRES(mutex) > +{ > + struct psample_exporter_map_node *node; Extra newline. > + HMAP_FOR_EACH_WITH_HASH (node, node, > + hash_int(collector_set_id, 0), > + &ps->exporters_map) { > + if (node->exporter.collector_set_id == collector_set_id) { > + return node; > + } > + } > + return NULL; > +} > + > +/* Configuration. */ > + > +/* Sets the psample configuration. > + * Returns true if the configuration has changed. */ > +bool > +dpif_psample_set_options(struct dpif_psample *ps, > + const struct ovs_list *options_list) Not sure if a ovs_list is the best way to pass these options around, but anyhow maybe the name should be a reference to the list structure to get a quick reference, i.e., ofproto_???_options? > +OVS_EXCLUDED(mutex) > +{ > + struct ofproto_psample_options *options; > + struct psample_exporter_map_node *node; > + bool changed = false; > + > + ovs_mutex_lock(&mutex); > + > + /* psample exporters do not hold any runtime memory so we do not need to > + * be extra careful at detecting which exporter changed and which did > + * not. As soon as we detect any change we can just recreate them all. */ Double space for new lines. Also not sure what multi line comment style we would like to enforce for new files. Coding style prefers /* and /* on new lines, but other options are allowed. Ilya? > + LIST_FOR_EACH(options, list_node, options_list) { > + node = dpif_psample_find_exporter_node(ps, > options->collector_set_id); > + if (!node || > + node->exporter.collector_set_id != options->collector_set_id || > + node->exporter.group_id != options->group_id) { > + changed = true; > + break; > + } > + } > + changed |= (hmap_count(&ps->exporters_map) != > ovs_list_size(options_list)); Maybe do this check first, so the list walk can be avoided? > + > + if (changed) { > + dpif_psample_clear(ps); > + LIST_FOR_EACH(options, list_node, options_list) { > + dpif_psample_new_exporter_node(ps, options); > + } > + } > + > + ovs_mutex_unlock(&mutex); > + > + return changed; > +} > + > +/* Creation and destruction. */ New line? > +struct dpif_psample * > +dpif_psample_create(void) > +{ > + struct dpif_psample *ps; New line? > + ps = xzalloc(sizeof *ps); > + hmap_init(&ps->exporters_map); > + ovs_refcount_init(&ps->ref_cnt); > + return ps; > +} > + > +static void > +dpif_psample_destroy(struct dpif_psample *ps) OVS_EXCLUDED(mutex) > +{ > + if (ps) { > + ovs_mutex_lock(&mutex); > + dpif_psample_clear(ps); > + free(ps); > + ovs_mutex_unlock(&mutex); > + } > +} > + > +/* Reference counting. */ > +struct dpif_psample* > +dpif_psample_ref(const struct dpif_psample *ps_) > +{ > + struct dpif_psample *ps = CONST_CAST(struct dpif_psample*, ps_); > + if (ps) { > + ovs_refcount_ref(&ps->ref_cnt); > + } > + return ps; > +} > + > +void > +dpif_psample_unref(struct dpif_psample *ps) OVS_EXCLUDED(mutex) > +{ > + if (ps && ovs_refcount_unref_relaxed(&ps->ref_cnt) == 1) { I think both the unref() functions need the mutex, after ovs_refcount_unref_relaxed() and before calling dpif_psample_destroy() someone could have called ref(). Or you need to add another ref check to dpif_sample_destroy(). For dpif_psample_ref() you need the lock, or be sure you already hold a reference count for the object. Not sure how we can force that somehow, but we should add some comment to the API. > + dpif_psample_destroy(ps); > + } > +} > diff --git a/ofproto/ofproto-dpif-psample.h b/ofproto/ofproto-dpif-psample.h > new file mode 100644 > index 000000000..80ba44fb9 > --- /dev/null > +++ b/ofproto/ofproto-dpif-psample.h > @@ -0,0 +1,31 @@ > +/* > + * 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_PSAMPLE_H > +#define OFPROTO_DPIF_PSAMPLE_H 1 > + > +#include <stdbool.h> > + > +struct dpif_psample; > +struct ovs_list; I think including stdbool.h and ovs_list can be omited, by just adding the correct includes in the C files, i.e.: diff --git a/ofproto/ofproto-dpif-psample.c b/ofproto/ofproto-dpif-psample.c index a83530ed8..d00b9be16 100644 --- a/ofproto/ofproto-dpif-psample.c +++ b/ofproto/ofproto-dpif-psample.c @@ -15,14 +15,17 @@ */ #include <config.h> -#include "ofproto-dpif-psample.h" +#include <stdbool.h> #include "hash.h" #include "ofproto.h" #include "openvswitch/hmap.h" +#include "openvswitch/list.h" #include "openvswitch/thread.h" #include "openvswitch/vlog.h" +#include "ofproto-dpif-psample.h" > + > +struct dpif_psample *dpif_psample_create(void); > +void dpif_psample_unref(struct dpif_psample *); > +struct dpif_psample* dpif_psample_ref(const struct dpif_psample *); struct dpif_psample *dpif_psample_ref > + > +bool dpif_psample_set_options(struct dpif_psample *, const struct ovs_list > *); We should have the name of the ovs_list argument, so we have a clue what kind if list we are passing, i.e. ofproto_???_options? > + > +#endif // OFPROTO_DPIF_PSAMPLE_H We do not use // for comments, use /* OFPROTO_DPIF_PSAMPLE_H */ instead. > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 3cee2795a..fbb83a9ec 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -50,6 +50,7 @@ > #include "ofproto-dpif-sflow.h" > #include "ofproto-dpif-trace.h" > #include "ofproto-dpif-upcall.h" > +#include "ofproto-dpif-psample.h" > #include "ofproto-dpif-xlate.h" > #include "ofproto-dpif-xlate-cache.h" > #include "openvswitch/ofp-actions.h" > @@ -2529,6 +2530,37 @@ get_ipfix_stats(const struct ofproto *ofproto_, > return dpif_ipfix_get_stats(di, bridge_ipfix, replies); > } > > +static int > +set_psample(struct ofproto *ofproto_, const struct ovs_list *pso) see other places about pso argument name. > +{ > + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); > + struct dpif_psample *ps = ofproto->psample; > + bool changed = false; > + bool has_options = pso && !ovs_list_is_empty(pso); reverse Christmas tree? > + > + if(!ofproto->backer->rt_support.psample) Space after ( > + return ENOTSUP; Always use {} with OVS, same below. > + > + if (has_options && !ps) { > + ps = ofproto->psample = dpif_psample_create(); > + changed = true; > + } > + > + if (ps) { > + if (!has_options) { > + dpif_psample_unref(ps); > + ofproto->psample = NULL; Maybe we can do ps = ofproto->psample = NULL; this will probably prevent code getting pushed in below trying to access ps? > + changed = true; > + } else { > + changed |= dpif_psample_set_options(ps, pso); > + } > + } > + > + if (changed) > + ofproto->backer->need_revalidate = REV_RECONFIGURE; New line. > + return 0; > +} > + > static int > set_cfm(struct ofport *ofport_, const struct cfm_settings *s) > { > @@ -7099,6 +7131,7 @@ const struct ofproto_class ofproto_dpif_class = { > get_netflow_ids, > set_sflow, > set_ipfix, > + set_psample, > get_ipfix_stats, > set_cfm, > cfm_status_changed, > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h > index 3db4263c7..97b2e78f1 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_psample *psample; Maybe fix the reverse Christmas tree while you’re at it. > 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..2ed7ebfbe 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 psample on 'ofproto' according to the options in > + * 'options' or turns off psample if 'options' is NULL. > + * > + * 'options' contains 'struct ofproto_psample_options'. > + * EOPNOTSUPP as a return value indicates that 'ofproto' does not support > + * psample, as does a null pointer. */ > + int (*set_psample)(struct ofproto *ofproto, > + const struct ovs_list *options); I think we should pass on an array rather than a list like ofproto_ipfix_flow_exporter_options() this is more cache friendly, and also avoid small memory allocations. > + > /* 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..22f584740 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -1000,6 +1000,16 @@ ofproto_get_datapath_cap(const char *datapath_type, > struct smap *dp_cap) > } > } > > +int ofproto_set_psample(struct ofproto *ofproto, > + const struct ovs_list *pso) > +{ > + if (ofproto->ofproto_class->set_psample) { > + return ofproto->ofproto_class->set_psample(ofproto, pso); > + } else { > + return (!pso || ovs_list_is_empty(pso)) ? 0: ENOTSUP; This looks odd, but others do the same :( See comment in later patch. > + } > +} > + > /* 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..dc1c470dd 100644 > --- a/ofproto/ofproto.h > +++ b/ofproto/ofproto.h > @@ -103,6 +103,12 @@ struct ofproto_ipfix_flow_exporter_options { > char *virtual_obs_id; > }; > > +struct ofproto_psample_options { > + struct ovs_list list_node; > + 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 +396,8 @@ 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_psample(struct ofproto *, > + const struct ovs_list *); We should have the name of the ovs_list argument, so we have a clue what kind if list we are passing, i.e. ofproto_???_options? > > /* Configuration of ports. */ > void ofproto_port_unregister(struct ofproto *, ofp_port_t ofp_port); > -- > 2.44.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
