| Message ID | 20240424195337.3596657-4-amorenoz@redhat.com |
|---|---|
| State | RFC |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | Add psample support to NXAST_SAMPLE action. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/intel-ovs-compilation | fail | test: fail |
| ovsrobot/apply-robot | warning | apply and check: warning |
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 <amorenoz@redhat.com> > --- > 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
On 5/10/24 12:06, 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 <amorenoz@redhat.com> >> --- >> 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 <snip> >> +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? Coding style allows for /* and */ on the same line as the comment start/end, and I prefer it that way. Putting them on separate lines sometimes beneficial for readability when the code is busy and tightly packed, so enforcing one way or another is probably not a great thing. Best regards, Ilya Maximets.
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 <amorenoz@redhat.com> >> --- >> 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 >> + >> +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 >
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 <amorenoz@redhat.com> >>> --- >>> 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 >>
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 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); + +static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER; + +struct psample_exporter { + uint32_t group_id; + uint32_t collector_set_id; +}; + +struct psample_exporter_map_node { + struct hmap_node node; + struct psample_exporter exporter; +}; + +struct dpif_psample { + struct hmap exporters_map; /* Contains psample_exporter_map_node. */ + + struct ovs_refcount ref_cnt; +}; + +/* Exporter handling */ +static void +dpif_psample_clear(struct dpif_psample *ps) OVS_REQUIRES(mutex) +{ + 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, + 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; + 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; + 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) +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. */ + 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)); + + 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. */ +struct dpif_psample * +dpif_psample_create(void) +{ + struct dpif_psample *ps; + 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) { + 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; + +struct dpif_psample *dpif_psample_create(void); +void dpif_psample_unref(struct dpif_psample *); +struct dpif_psample* dpif_psample_ref(const struct dpif_psample *); + +bool dpif_psample_set_options(struct dpif_psample *, const struct ovs_list *); + +#endif // OFPROTO_DPIF_PSAMPLE_H 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) +{ + 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); + + if(!ofproto->backer->rt_support.psample) + return ENOTSUP; + + if (has_options && !ps) { + ps = ofproto->psample = dpif_psample_create(); + changed = true; + } + + if (ps) { + if (!has_options) { + dpif_psample_unref(ps); + ofproto->psample = NULL; + changed = true; + } else { + changed |= dpif_psample_set_options(ps, pso); + } + } + + if (changed) + ofproto->backer->need_revalidate = REV_RECONFIGURE; + 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; 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); + /* 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; + } +} + /* 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 *); /* Configuration of ports. */ void ofproto_port_unregister(struct ofproto *, ofp_port_t ofp_port);
Add a new resource in ofproto-dpif and the corresponding API in ofproto_provider.h to represent and change psample configuration. Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- 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