> -----Original Message-----
> From: Amber, Kumar <kumar.am...@intel.com>
> Sent: Sunday 27 March 2022 08:09
> To: ovs-dev@openvswitch.org
> Cc: Stokes, Ian <ian.sto...@intel.com>; echau...@redhat.com;
> ktray...@redhat.com; i.maxim...@ovn.org;
> Ferriter, Cian <cian.ferri...@intel.com>; f...@sysclose.org; Van Haaren, Harry
> <harry.van.haa...@intel.com>; Amber, Kumar <kumar.am...@intel.com>
> Subject: [PATCH v2 3/4] Miniflow_extract: Refactor miniflow_extract into api.
I had a look at previous since the 'miniflow_extract:' commit title beginning
looked strange. Commits which change the actual miniflow_extract() function
code or surrounding code typically have 'flow:' as the beginning of the commit
title. Can we use that?
>
> Miniflow extract used to takes the ABI parameter struct
> miniflow which was removed and added inside
> the struct netdev_flow_key and at many places temperory
> structs were created inside the functions which could be
> cleaned in favour of a uniform API.
>
> Signed-off-by: Kumar Amber <kumar.am...@intel.com>
> ---
> lib/dpif-netdev-avx512.c | 2 +-
> lib/dpif-netdev-private-extract.c | 2 +-
> lib/dpif-netdev.c | 2 +-
> lib/flow.c | 28 +++++++++++++++++++---------
> lib/flow.h | 6 +++++-
> ofproto/ofproto.c | 10 ++++------
> 6 files changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index b7131ba3f..76eeecc9a 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -211,7 +211,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
> *pmd,
>
> if (!mfex_hit) {
> /* Do a scalar miniflow extract into keys. */
> - miniflow_extract(packet, &key->mf);
> + miniflow_extract(packet, key);
> }
>
> /* Cache TCP and byte values for all packets. */
> diff --git a/lib/dpif-netdev-private-extract.c
> b/lib/dpif-netdev-private-extract.c
> index 4b2f12015..8538d069f 100644
> --- a/lib/dpif-netdev-private-extract.c
> +++ b/lib/dpif-netdev-private-extract.c
> @@ -251,7 +251,7 @@ dpif_miniflow_extract_autovalidator(struct
> dp_packet_batch *packets,
> /* Run scalar miniflow_extract to get default result. */
> DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> pkt_metadata_init(&packet->md, in_port);
> - miniflow_extract(packet, &keys[i].mf);
> + miniflow_extract_(packet, &keys[i]);
>
> /* Store known good metadata to compare with optimized metadata. */
> good_l2_5_ofs[i] = packet->l2_5_ofs;
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 5fc68bdbe..fd3fe510d 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -8144,7 +8144,7 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd,
> }
> }
>
> - miniflow_extract(packet, &key->mf);
> + miniflow_extract(packet, key);
> key->len = 0; /* Not computed yet. */
> key->hash =
> (md_is_valid == false)
> diff --git a/lib/flow.c b/lib/flow.c
> index dd523c889..127de2d7a 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -35,6 +35,8 @@
> #include "jhash.h"
> #include "openvswitch/match.h"
> #include "dp-packet.h"
> +#include "dpif-netdev-private-thread.h"
Why is this header include added as part of this patch?
> +#include "dpif-netdev-private-dpcls.h"
> #include "openflow/openflow.h"
> #include "packets.h"
> #include "odp-util.h"
> @@ -633,15 +635,18 @@ parse_nsh(const void **datap, size_t *sizep, struct
> ovs_key_nsh *key)
> void
> flow_extract(struct dp_packet *packet, struct flow *flow)
> {
> - struct {
> - struct miniflow mf;
> - uint64_t buf[FLOW_U64S];
> - } m;
This is the value add of this refactor. Not having multiple places in the code
where we build a "netdev_flow_key like" struct is good IMO.
But it comes at the cost of including a extra header in those places. I think
it's worth it.
> +
> + struct netdev_flow_key key;
>
> COVERAGE_INC(flow_extract);
>
> - miniflow_extract(packet, &m.mf);
> - miniflow_expand(&m.mf, flow);
> + /* Changing parameter to key will not affect anything as
> + * buff array is still followed by the mf bit map inside
> + * netdev_flow_key, thus there wont be any impact on offset
> + * calculations which were done earlier.
> + */
This seems like a comment to explain why you are making a change. Comments in
the code should explain why things are done a certain way, not why something
was changed from one way to another way. That type of explanation belongs in a
commit message.
> + miniflow_extract(packet, &key);
> + miniflow_expand(&key.mf, flow);
> }
>
> static inline bool
> @@ -758,7 +763,7 @@ dump_invalid_packet(struct dp_packet *packet, const char
> *reason)
> * of interest for the flow, otherwise UINT16_MAX.
> */
> void
> -miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
> +miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key *key)
> {
> /* Add code to this function (or its callees) to extract new fields. */
> BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42);
> @@ -767,7 +772,7 @@ miniflow_extract(struct dp_packet *packet, struct
> miniflow *dst)
> const void *data = dp_packet_data(packet);
> size_t size = dp_packet_size(packet);
> ovs_be32 packet_type = packet->packet_type;
> - uint64_t *values = miniflow_values(dst);
> + uint64_t *values = miniflow_values(&key->mf);
> struct mf_ctx mf = { FLOWMAP_EMPTY_INITIALIZER, values,
> values + FLOW_U64S };
> const char *frame;
> @@ -1110,9 +1115,14 @@ miniflow_extract(struct dp_packet *packet, struct
> miniflow *dst)
> }
> }
> out:
> - dst->map = mf.map;
> + key->mf.map = mf.map;
> }
>
> +void
> +miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key)
> +{
> + miniflow_extract_(packet, key);
> +}
I think this should be part of the next commit. This commit could be solely
about changing the 2nd argument of miniflow_extract() from 'struct miniflow' to
'struct netdev_flow_key'.
This change should be part of the next commit.
> static ovs_be16
> parse_dl_type(const void **datap, size_t *sizep, ovs_be16 *first_vlan_tci_p)
> {
> diff --git a/lib/flow.h b/lib/flow.h
> index c647ad83c..3bfe12f31 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -41,6 +41,7 @@ struct dp_packet;
> struct ofputil_port_map;
> struct pkt_metadata;
> struct match;
> +struct netdev_flow_key;
>
> /* Some flow fields are mutually exclusive or only appear within the flow
> * pipeline. IPv6 headers are bigger than IPv4 and MPLS, and IPv6 ND packets
> @@ -540,7 +541,10 @@ struct pkt_metadata;
> /* The 'dst' must follow with buffer space for FLOW_U64S 64-bit units.
> * 'dst->map' is ignored on input and set on output to indicate which fields
> * were extracted. */
> -void miniflow_extract(struct dp_packet *packet, struct miniflow *dst);
> +void
> +miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key);
> +void
> +miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key *key);
> void miniflow_map_init(struct miniflow *, const struct flow *);
> void flow_wc_map(const struct flow *, struct flowmap *);
> size_t miniflow_alloc(struct miniflow *dsts[], size_t n,
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 7e09a588a..43de229c3 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -30,6 +30,7 @@
> #include "connmgr.h"
> #include "coverage.h"
> #include "dp-packet.h"
> +#include "dpif-netdev-private-dpcls.h"
> #include "hash.h"
> #include "openvswitch/hmap.h"
> #include "netdev.h"
> @@ -3629,10 +3630,7 @@ ofproto_packet_out_init(struct ofproto *ofproto,
> {
> enum ofperr error;
> struct match match;
> - struct {
> - struct miniflow mf;
> - uint64_t buf[FLOW_U64S];
> - } m;
> + struct netdev_flow_key key;
>
> uint16_t in_port = ofp_to_u16(po->flow_metadata.flow.in_port.ofp_port);
> if (in_port >= ofproto->max_ports && in_port < ofp_to_u16(OFPP_MAX)) {
> @@ -3653,8 +3651,8 @@ ofproto_packet_out_init(struct ofproto *ofproto,
> /* Store struct flow. */
> opo->flow = xmalloc(sizeof *opo->flow);
> *opo->flow = po->flow_metadata.flow;
> - miniflow_extract(opo->packet, &m.mf);
> - flow_union_with_miniflow(opo->flow, &m.mf);
> + miniflow_extract(opo->packet, &key);
> + flow_union_with_miniflow(opo->flow, &key.mf);
>
> /* Check actions like for flow mods. We pass a 'table_id' of 0 to
> * ofproto_check_consistency(), which isn't strictly correct because
> these
> --
> 2.25.1
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
