On 7/9/24 15:32, Adrián Moreno wrote: > On Tue, Jul 09, 2024 at 11:45:19AM GMT, Eelco Chaudron wrote: >> On 7 Jul 2024, at 22:08, Adrian Moreno wrote: >> >>> Add a new resource in ofproto-dpif and the corresponding API in >>> ofproto_provider.h to represent and local sampling configuration. >>> >>> Signed-off-by: Adrian Moreno <amore...@redhat.com> >> >> Some small nits below. >> >> //Eelco >> >>> --- >>> ofproto/automake.mk | 2 + >>> ofproto/ofproto-dpif-lsample.c | 187 +++++++++++++++++++++++++++++++++ >>> ofproto/ofproto-dpif-lsample.h | 36 +++++++ >>> ofproto/ofproto-dpif.c | 38 +++++++ >>> ofproto/ofproto-dpif.h | 1 + >>> ofproto/ofproto-provider.h | 9 ++ >>> ofproto/ofproto.c | 12 +++ >>> ofproto/ofproto.h | 8 ++ >>> 8 files changed, 293 insertions(+) >>> create mode 100644 ofproto/ofproto-dpif-lsample.c >>> create mode 100644 ofproto/ofproto-dpif-lsample.h >>> >>> diff --git a/ofproto/automake.mk b/ofproto/automake.mk >>> index 7c08b563b..cb1361b8a 100644 >>> --- a/ofproto/automake.mk >>> +++ b/ofproto/automake.mk >>> @@ -30,6 +30,8 @@ ofproto_libofproto_la_SOURCES = \ >>> ofproto/ofproto-dpif.h \ >>> ofproto/ofproto-dpif-ipfix.c \ >>> ofproto/ofproto-dpif-ipfix.h \ >>> + ofproto/ofproto-dpif-lsample.c \ >>> + ofproto/ofproto-dpif-lsample.h \ >>> ofproto/ofproto-dpif-mirror.c \ >>> ofproto/ofproto-dpif-mirror.h \ >>> ofproto/ofproto-dpif-monitor.c \ >>> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c >>> new file mode 100644 >>> index 000000000..d675a116f >>> --- /dev/null >>> +++ b/ofproto/ofproto-dpif-lsample.c >>> @@ -0,0 +1,187 @@ >>> +/* >>> + * Copyright (c) 2024 Red Hat, Inc. >>> + * >>> + * Licensed under the Apache License, Version 2.0 (the "License"); >>> + * you may not use this file except in compliance with the License. >>> + * You may obtain a copy of the License at: >>> + * >>> + * http://www.apache.org/licenses/LICENSE-2.0 >>> + * >>> + * Unless required by applicable law or agreed to in writing, software >>> + * distributed under the License is distributed on an "AS IS" BASIS, >>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. >>> + * See the License for the specific language governing permissions and >>> + * limitations under the License. >>> + */ >>> + >>> +#include <config.h> >>> +#include "ofproto-dpif-lsample.h" >>> + >>> +#include "cmap.h" >>> +#include "hash.h" >>> +#include "ofproto.h" >>> +#include "openvswitch/thread.h" >>> + >>> +/* Dpif local sampling. >>> + * >>> + * Thread safety: dpif_lsample allows lockless concurrent reads of local >>> + * sampling exporters as long as the following restrictions are met: >>> + * 1) While the last reference is being dropped, i.e: a thread is calling >>> + * "dpif_lsample_unref" on the last reference, other threads cannot >>> call >>> + * "dpif_lsample_ref". >>> + * 2) Threads do not quiese while holding references to internal >>> + * lsample_exporter objects. >>> + */ >>> + >>> +struct dpif_lsample { >>> + struct cmap exporters; /* Contains lsample_exporter_node >>> instances >>> + indexed by collector_set_id. */ >>> + struct ovs_mutex mutex; /* Protects concurrent >>> insertion/deletion >>> + * of exporters. */ >>> + >>> + struct ovs_refcount ref_cnt; /* Controls references to this >>> instance. */ >>> +}; >>> + >>> +struct lsample_exporter { >>> + struct ofproto_lsample_options options; >>> +}; >>> + >>> +struct lsample_exporter_node { >>> + struct cmap_node node; /* In dpif_lsample->exporters. */ >>> + struct lsample_exporter exporter; >>> +}; >>> + >>> +static void >>> +dpif_lsample_delete_exporter(struct dpif_lsample *lsample, >>> + struct lsample_exporter_node *node) >>> +{ >>> + ovs_mutex_lock(&lsample->mutex); >>> + cmap_remove(&lsample->exporters, &node->node, >>> + hash_int(node->exporter.options.collector_set_id, 0)); >>> + ovs_mutex_unlock(&lsample->mutex); >>> + >>> + ovsrcu_postpone(free, node); >>> +} >>> + >>> +/* Adds an exporter with the provided options which are copied. */ >>> +static struct lsample_exporter_node * >>> +dpif_lsample_add_exporter(struct dpif_lsample *lsample, >>> + const struct ofproto_lsample_options *options) >>> +{ >>> + struct lsample_exporter_node *node; >>> + >>> + node = xzalloc(sizeof *node); >>> + node->exporter.options = *options; >>> + >>> + ovs_mutex_lock(&lsample->mutex); >>> + cmap_insert(&lsample->exporters, &node->node, >>> + hash_int(options->collector_set_id, 0)); >>> + ovs_mutex_unlock(&lsample->mutex); >>> + >>> + return node; >>> +} >>> + >>> +static struct lsample_exporter_node * >>> +dpif_lsample_find_exporter_node(const struct dpif_lsample *lsample, >>> + const uint32_t collector_set_id) >>> +{ >>> + struct lsample_exporter_node *node; >>> + >>> + CMAP_FOR_EACH_WITH_HASH (node, node, >>> + hash_int(collector_set_id, 0), >>> + &lsample->exporters) { >>> + if (node->exporter.options.collector_set_id == collector_set_id) { >>> + return node; >>> + } >>> + } >>> + return NULL; >>> +} >>> + >>> +/* Sets the lsample configuration and returns true if the configuration >>> + * has changed. */ >>> +bool >>> +dpif_lsample_set_options(struct dpif_lsample *lsample, >>> + const struct ofproto_lsample_options *options, >>> + size_t n_options) >>> +{ >>> + const struct ofproto_lsample_options *opt; >>> + struct lsample_exporter_node *node; >>> + bool changed = false; >>> + int i; >>> + >>> + for (i = 0; i < n_options; i++) { >>> + opt = &options[i]; >>> + node = dpif_lsample_find_exporter_node(lsample, >>> + opt->collector_set_id); >>> + if (!node) { >>> + dpif_lsample_add_exporter(lsample, opt); >>> + changed = true; >>> + } else if (memcmp(&node->exporter.options, opt, sizeof(*opt))) { >>> + dpif_lsample_delete_exporter(lsample, node); >>> + dpif_lsample_add_exporter(lsample, opt); >>> + changed = true; >>> + } >>> + } >>> + >>> + /* Delete exporters that have been removed. */ >>> + CMAP_FOR_EACH (node, node, &lsample->exporters) { >>> + for (i = 0; i < n_options; i++) { >>> + if (node->exporter.options.collector_set_id >>> + == options[i].collector_set_id) { >>> + break; >>> + } >>> + } >>> + if (i == n_options) { >>> + dpif_lsample_delete_exporter(lsample, node); >>> + changed = true; >>> + } >>> + } >>> + >>> + return changed; >>> +} >>> + >>> +struct dpif_lsample * >>> +dpif_lsample_create(void) >>> +{ >>> + struct dpif_lsample *lsample; >>> + >>> + lsample = xzalloc(sizeof *lsample); >>> + cmap_init(&lsample->exporters); >>> + ovs_mutex_init(&lsample->mutex); >>> + ovs_refcount_init(&lsample->ref_cnt); >>> + >>> + return lsample; >>> +} >>> + >>> +static void >>> +dpif_lsample_destroy(struct dpif_lsample *lsample) >>> +{ >>> + if (lsample) { >>> + struct lsample_exporter_node *node; >>> + >>> + CMAP_FOR_EACH (node, node, &lsample->exporters) { >>> + dpif_lsample_delete_exporter(lsample, node); >>> + } >>> + cmap_destroy(&lsample->exporters); >>> + free(lsample); >>> + } >>> +} >>> + >>> +struct dpif_lsample * >>> +dpif_lsample_ref(const struct dpif_lsample *lsample_) >>> +{ >>> + struct dpif_lsample *lsample = CONST_CAST(struct dpif_lsample *, >>> lsample_); >>> + >>> + if (lsample) { >>> + ovs_refcount_ref(&lsample->ref_cnt); >>> + } >>> + return lsample; >>> +} >>> + >>> +void >>> +dpif_lsample_unref(struct dpif_lsample *lsample) >>> +{ >>> + if (lsample && ovs_refcount_unref_relaxed(&lsample->ref_cnt) == 1) { >>> + dpif_lsample_destroy(lsample); >>> + } >>> +} >>> diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h >>> new file mode 100644 >>> index 000000000..bee36c9c5 >>> --- /dev/null >>> +++ b/ofproto/ofproto-dpif-lsample.h >>> @@ -0,0 +1,36 @@ >>> +/* >>> + * Copyright (c) 2024 Red Hat, Inc. >>> + * >>> + * Licensed under the Apache License, Version 2.0 (the "License"); >>> + * you may not use this file except in compliance with the License. >>> + * You may obtain a copy of the License at: >>> + * >>> + * http://www.apache.org/licenses/LICENSE-2.0 >>> + * >>> + * Unless required by applicable law or agreed to in writing, software >>> + * distributed under the License is distributed on an "AS IS" BASIS, >>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. >>> + * See the License for the specific language governing permissions and >>> + * limitations under the License. >>> + */ >>> + >>> +#ifndef OFPROTO_DPIF_LSAMPLE_H >>> +#define OFPROTO_DPIF_LSAMPLE_H 1 >>> + >>> +#include <stdbool.h> >>> +#include <stdlib.h> >>> + >>> +struct dpif_lsample; >>> +struct ofproto_lsample_options; >>> + >>> +struct dpif_lsample *dpif_lsample_create(void); >>> + >>> +void dpif_lsample_unref(struct dpif_lsample *); >>> + >>> +struct dpif_lsample *dpif_lsample_ref(const struct dpif_lsample *); >> >> I would define ref before unref. >> >>> + >>> +bool dpif_lsample_set_options(struct dpif_lsample *, >>> + const struct ofproto_lsample_options *, >>> + size_t n_opts); >> >> Also not sure what would be the right way to group, not group, add extra >> new lines in include files. Looks like we sort of group similar function, >> and add new lines between groups. If we follow that it should probably look >> something like this: >> >> struct dpif_lsample *dpif_lsample_create(void); >> >> struct dpif_lsample *dpif_lsample_ref(const struct dpif_lsample *); >> void dpif_lsample_unref(struct dpif_lsample *); >> >> bool dpif_lsample_set_options(struct dpif_lsample *, >> const struct ofproto_lsample_options *, >> size_t n_opts); >> >> But I have no real preferences, maybe Ilya has. >> > > I don't either, your sorting looks good.
Grouping makes sense to me. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
