On 13 May 2024, at 8:45, Adrian Moreno wrote:
> On 5/10/24 12:06 PM, Eelco Chaudron wrote: >> 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? >> > > I agree, in the RFC-level, I just wrote this quick&dirty file to get the > feature working. I agree locking can be improved significantly. > > > >> 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() >> > > _free() would indicate it frees the struct which is not true. > *_delete_exporter is better. > > >> 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. >> > > IIUC, that means having the same structure for the API and for internal > configuration storage. While that makes code cleaner and avoids an > allocation, it ties both cases too much IMHO making it more difficult for > them to evolve seperately. OTOH, I don't see a need for this happening in the > short term... > >>> + 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. >> > > Ack. > >>> + 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? > > As mentioned in the other patch, I'm ok making it a struct. > >> >>> +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? >> > > Ack. > >>> + >>> + 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" >> > > If I understand your suggestion properly, I think it contradicts the Coding > Style that says: > > """ > #include directives should appear in the following order: > > 1. #include <config.h> > > 2. The module’s own headers, if any. Including this before any other > header (besides ) ensures that the module’s header file is self-contained > (see header files below). > > [...] > > Header files should be self-contained; that is, they should #include whatever > additional headers are required, without requiring the client to #include > them for it. > """ > > https://docs.openvswitch.org/en/latest/internals/contributing/coding-style/#source-files Thanks for quoting this ;) I guess nothing needs to change here... >>> + >>> +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? >> > > Ack. > >>> + >>> +#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. >> > > Ack. > >>> + >>> + 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? >> > > That's a nice trick, yes. > >>> + 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. >> > > OK. > >>> 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. >> > > OK. I'll change it in the next RFC version. > >>> + >>> /* 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. >> > > I agree. I'll move all the error masking to the client. > > >>> + } >>> +} >>> + >>> /* 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? >> > > I'll change it to an array which will be clearer as it'll be a pointer to the > options struct. > > >>> >>> /* 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
