git sha: 0f05b76a91c2930478db76bd4f49e897fbbe3e18
Author: Eelco Chaudron <[email protected]>
Subject: dpif-offload: Add flow dump APIs to dpif-offload.
This commit introduces an API in dpif-offload to dump all offloaded flows,
centralizing flow retrieval and intended to eventually replace the existing
netdev-offload flow dump mechanism.
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 2a700f24a..1411c5a31 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -964,10 +964,10 @@ determine_dpif_flow_dump_types(struct dump_types
> *dump_types,
> struct dpif_flow_dump_types *dpif_dump_types)
> {
> dpif_dump_types->ovs_flows = dump_types->ovs ||
> dump_types->non_offloaded;
> - dpif_dump_types->netdev_flows = dump_types->tc || dump_types->offloaded
> - || dump_types->non_offloaded
> - || dump_types->dpdk
> - || dump_types->partially_offloaded;
> + dpif_dump_types->offloaded_flows = dump_types->tc ||
> dump_types->offloaded
> + || dump_types->non_offloaded
> + || dump_types->dpdk
> + || dump_types->partially_offloaded;
> }
[ ... ]
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index e99807782..91e7ddcfa 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -65,23 +65,52 @@ static inline void dpif_assert_class(const struct dpif
> *dpif,
> struct dpif_flow_dump {
> struct dpif *dpif;
> bool terse; /* If true, key/mask/actions may be omitted. */
> +
> + struct ovs_mutex offload_dump_mutex;
> + struct dpif_offload_flow_dump **offload_dumps;
> + size_t n_offload_dumps;
> + size_t offload_dump_index;
> };
>
> +void dpif_offload_flow_dump_create(struct dpif_flow_dump *,
> + const struct dpif *, bool terse);
> +void dpif_offload_flow_dump_thread_create(struct dpif_flow_dump_thread *,
> + struct dpif_flow_dump *);
> +
> static inline void
> -dpif_flow_dump_init(struct dpif_flow_dump *dump, const struct dpif *dpif)
> +dpif_flow_dump_init(struct dpif_flow_dump *dump, const struct dpif *dpif,
> + bool terse, struct dpif_flow_dump_types *types)
> {
> dump->dpif = CONST_CAST(struct dpif *, dpif);
> + dump->terse = terse;
> + dump->offload_dumps = NULL;
> + dump->n_offload_dumps = 0;
> + dump->offload_dump_index = 0;
> + ovs_mutex_init(&dump->offload_dump_mutex);
> + if (!types || types->offloaded_flows) {
> + dpif_offload_flow_dump_create(dump, dpif, terse);
> + }
> }
What happens to the dump->offload_dump_mutex if
dpif_offload_flow_dump_create() fails after this inline function
initializes it? The mutex will remain initialized but might not be
properly cleaned up.
> diff --git a/lib/dpif.c b/lib/dpif.c
> index d6eea8574..2bc03a398 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1115,7 +1115,15 @@ int
> dpif_flow_dump_destroy(struct dpif_flow_dump *dump)
> {
> const struct dpif *dpif = dump->dpif;
> - int error = dpif->dpif_class->flow_dump_destroy(dump);
> + int error;
> + int offload_error;
> +
> + offload_error = dpif_offload_flow_dump_destroy(dump);
> + error = dpif->dpif_class->flow_dump_destroy(dump);
> +
> + if (!error || error == EOF) {
> + error = offload_error;
> + }
This error handling logic appears incomplete. If dpif->flow_dump_destroy()
fails with an error other than EOF, should the offload_error be ignored
entirely? Also, will the offload_dump_mutex be properly destroyed in all
error paths?
[ ... ]
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index 12dac4239..04435a919 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -832,6 +832,183 @@ dpif_offload_meter_del(const struct dpif *dpif,
> ofproto_meter_id meter_id,
> }
> }
>
> +void
> +dpif_offload_flow_dump_create(struct dpif_flow_dump *dump,
> + const struct dpif *dpif, bool terse)
> +{
> + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif);
> + const struct dpif_offload *offload;
> + size_t n_providers = 0;
> + int i = 0;
> +
> + if (!dump || !dpif_offload_is_offload_enabled() || !dp_offload) {
> + return;
> + }
> +
> + LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) {
> + if (offload->class->flow_dump_create) {
> + n_providers++;
> + }
> + }
> +
> + if (!n_providers) {
> + return;
> + }
> +
> + dump->offload_dumps = xmalloc(n_providers * sizeof(
> + struct dpif_offload_flow_dump *));
> +
> + LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) {
> + if (offload->class->flow_dump_create) {
> + dump->offload_dumps[i++] = offload->class->flow_dump_create(
> + offload, terse);
> + }
> + }
> + dump->n_offload_dumps = i;
> + dump->offload_dump_index = 0;
> +}
Can this function leak dump->offload_dumps if any of the
flow_dump_create() calls fail? There's no cleanup handling if one of the
provider creation calls returns NULL or fails.
> +int
> +dpif_offload_flow_dump_destroy(struct dpif_flow_dump *dump)
> +{
> + int error = 0;
> +
> + for (int i = 0; i < dump->n_offload_dumps; i++) {
> + struct dpif_offload_flow_dump *offload_dump = dump->offload_dumps[i];
> + const struct dpif_offload *offload = offload_dump->offload;
> + int rc = offload->class->flow_dump_destroy(offload_dump);
> +
> + if (rc && rc != EOF) {
> + VLOG_ERR("Failed flow dumping on dpif-offload provider "
> + "%s, error %s", dpif_offload_name(offload),
> + ovs_strerror(rc));
> + if (!error) {
> + error = rc;
> + }
> + }
> + }
> + ovs_mutex_destroy(&dump->offload_dump_mutex);
> + free(dump->offload_dumps);
> + return error;
> +}
Will this function properly handle cases where dump->offload_dumps is
NULL? If dpif_offload_flow_dump_create() returns early, this function
might call free() on a NULL pointer, which is safe, but the
ovs_mutex_destroy() will still be called even if the mutex was never used.
[ ... ]
> +void
> +dpif_offload_flow_dump_thread_create(struct dpif_flow_dump_thread *thread,
> + struct dpif_flow_dump *dump)
> +{
> + if (!dpif_offload_is_offload_enabled() || !dump
> + || !dump->n_offload_dumps) {
> + return;
> + }
> +
> + thread->n_offload_threads = dump->n_offload_dumps;
> + thread->offload_dump_done = false;
> + thread->offload_dump_index = 0;
> + thread->offload_threads = xmalloc(thread->n_offload_threads * sizeof(
> + struct dpif_offload_flow_dump_thread *));
> +
> + for (int i = 0; i < dump->n_offload_dumps; i++) {
> + struct dpif_offload_flow_dump *offload_dump = dump->offload_dumps[i];
> + const struct dpif_offload *offload = offload_dump->offload;
> +
> + thread->offload_threads[i] = offload->class->flow_dump_thread_create(
> + offload_dump);
> + }
> +}
Does this code leak thread->offload_threads if any of the
flow_dump_thread_create() calls fail? There's no cleanup handling for
partial failures during thread creation.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
