On 16 Mar 2022, at 19:51, Mike Pattrick wrote:
> Currently we allocate and format a string in dp_netdev_flow_add in case
> miniflow_bits needs to be printed by dpctl/dump-flows/get-flow. However,
> this adds unneeded calls to realloc in the pmd hot path, while
> the resulting string may never be viewed.
>
> Instead of keeping a pointer to an allocated string, now we just keep
> track of the 16 byte flow map.
Harry can you take a look at this, as this was added as part of MFEX.
At a first glance, the only thing I see is that “dp-extra-info” is/was a
general dp type independent string, and now it has become something for a
specific dp and use case.
//Eelco
> Signed-off-by: Mike Pattrick <m...@redhat.com>
> ---
> lib/dpctl.c | 13 +++++++++++--
> lib/dpif-netdev-private-flow.h | 2 +-
> lib/dpif-netdev.c | 15 +--------------
> lib/dpif-netlink.c | 2 +-
> lib/dpif.h | 6 +++---
> lib/flow.h | 1 +
> lib/netdev-offload-dpdk.c | 2 +-
> lib/netdev-offload-tc.c | 2 +-
> 8 files changed, 20 insertions(+), 23 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 29041fa3e..6ff26fb1b 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -876,8 +876,17 @@ format_dpif_flow(struct ds *ds, const struct dpif_flow
> *f, struct hmap *ports,
> }
> ds_put_cstr(ds, ", actions:");
> format_odp_actions(ds, f->actions, f->actions_len, ports);
> - if (dpctl_p->verbosity && f->attrs.dp_extra_info) {
> - ds_put_format(ds, ", dp-extra-info:%s", f->attrs.dp_extra_info);
> + if (dpctl_p->verbosity && !flowmap_is_empty(f->attrs.dp_extra_info)) {
> + size_t unit;
> + ds_put_cstr(ds, ", dp-extra-info:miniflow_bits(");
> + FLOWMAP_FOR_EACH_UNIT (unit) {
> + if (unit) {
> + ds_put_char(ds, ',');
> + }
> + ds_put_format(ds, "%d",
> + count_1bits(f->attrs.dp_extra_info.bits[unit]));
> + }
> + ds_put_char(ds, ')');
> }
> }
>
> diff --git a/lib/dpif-netdev-private-flow.h b/lib/dpif-netdev-private-flow.h
> index 66016eb09..a99257251 100644
> --- a/lib/dpif-netdev-private-flow.h
> +++ b/lib/dpif-netdev-private-flow.h
> @@ -123,7 +123,7 @@ struct dp_netdev_flow {
> struct packet_batch_per_flow *batch;
>
> /* Packet classification. */
> - char *dp_extra_info; /* String to return in a flow dump/get. */
> + struct flowmap dp_extra_info;
> struct dpcls_rule cr; /* In owning dp_netdev's 'cls'. */
> /* 'cr' must be the last member. */
> };
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 720818e30..380274a3a 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2380,7 +2380,6 @@ static void
> dp_netdev_flow_free(struct dp_netdev_flow *flow)
> {
> dp_netdev_actions_free(dp_netdev_flow_get_actions(flow));
> - free(flow->dp_extra_info);
> free(flow);
> }
>
> @@ -4058,11 +4057,9 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
> odp_port_t orig_in_port)
> OVS_REQUIRES(pmd->flow_mutex)
> {
> - struct ds extra_info = DS_EMPTY_INITIALIZER;
> struct dp_netdev_flow *flow;
> struct netdev_flow_key mask;
> struct dpcls *cls;
> - size_t unit;
>
> /* Make sure in_port is exact matched before we read it. */
> ovs_assert(match->wc.masks.in_port.odp_port == ODPP_NONE);
> @@ -4106,17 +4103,7 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
> cls = dp_netdev_pmd_find_dpcls(pmd, in_port);
> dpcls_insert(cls, &flow->cr, &mask);
>
> - ds_put_cstr(&extra_info, "miniflow_bits(");
> - FLOWMAP_FOR_EACH_UNIT (unit) {
> - if (unit) {
> - ds_put_char(&extra_info, ',');
> - }
> - ds_put_format(&extra_info, "%d",
> - count_1bits(flow->cr.mask->mf.map.bits[unit]));
> - }
> - ds_put_char(&extra_info, ')');
> - flow->dp_extra_info = ds_steal_cstr(&extra_info);
> - ds_destroy(&extra_info);
> + flow->dp_extra_info = flow->cr.mask->mf.map;
>
> cmap_insert(&pmd->flow_table, CONST_CAST(struct cmap_node *,
> &flow->node),
> dp_netdev_flow_hash(&flow->ufid));
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 71e35ccdd..a63c6ac8f 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1768,7 +1768,7 @@ dpif_netlink_flow_to_dpif_flow(struct dpif_flow
> *dpif_flow,
> dpif_netlink_flow_get_stats(datapath_flow, &dpif_flow->stats);
> dpif_flow->attrs.offloaded = false;
> dpif_flow->attrs.dp_layer = "ovs";
> - dpif_flow->attrs.dp_extra_info = NULL;
> + dpif_flow->attrs.dp_extra_info = FLOWMAP_EMPTY_MAP;
> }
>
> /* The design is such that all threads are working together on the first dump
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 6cb4dae6d..6fd46ef94 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -515,9 +515,9 @@ struct dpif_flow_detailed_stats {
> };
>
> struct dpif_flow_attrs {
> - bool offloaded; /* True if flow is offloaded to HW. */
> - const char *dp_layer; /* DP layer the flow is handled in. */
> - const char *dp_extra_info; /* Extra information provided by DP. */
> + bool offloaded; /* True if flow is offloaded to HW. */
> + const char *dp_layer; /* DP layer the flow is handled in. */
> + struct flowmap dp_extra_info; /* Extra information provided by DP. */
> };
>
> struct dpif_flow_dump_types {
> diff --git a/lib/flow.h b/lib/flow.h
> index c647ad83c..5d8481add 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -283,6 +283,7 @@ struct flowmap {
> };
>
> #define FLOWMAP_EMPTY_INITIALIZER { { 0 } }
> +static const struct flowmap FLOWMAP_EMPTY_MAP = FLOWMAP_EMPTY_INITIALIZER;
>
> static inline void flowmap_init(struct flowmap *);
> static inline bool flowmap_equal(struct flowmap, struct flowmap);
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 94dc6a9b7..d18cbaa5c 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -2458,7 +2458,7 @@ netdev_offload_dpdk_flow_get(struct netdev *netdev,
> struct rte_flow_error error;
> int ret = 0;
>
> - attrs->dp_extra_info = NULL;
> + attrs->dp_extra_info = FLOWMAP_EMPTY_MAP;
>
> rte_flow_data = ufid_to_rte_flow_data_find(netdev, ufid, false);
> if (!rte_flow_data || !rte_flow_data->rte_flow ||
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 3f7068c8e..1d47eb624 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -600,7 +600,7 @@ parse_tc_flower_to_attrs(struct tc_flower *flower,
> flower->offloaded_state ==
> TC_OFFLOADED_STATE_UNDEFINED);
> attrs->dp_layer = "tc";
> - attrs->dp_extra_info = NULL;
> + attrs->dp_extra_info = FLOWMAP_EMPTY_MAP;
> }
>
> static int
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
