Message ID | 20220713182807.3416578-10-harry.van.haaren@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Actions Infrastructure + Optimizations | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | success | test: success |
Hey all,
Have tested the all the patches in the series.
<Snip>
Tested-by: Kumar Amber <kumar.amber@intel.com>
BR
Amber
> -----Original Message----- > From: Van Haaren, Harry <harry.van.haaren@intel.com> > Sent: Wednesday, July 13, 2022 11:58 PM > To: dev@openvswitch.org > Cc: i.maximets@ovn.org; echaudro@redhat.com; Amber, Kumar > <kumar.amber@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Finn, Emma > <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com> > Subject: [PATCH v10 09/10] odp-execute: Add ISA implementation of > set_masked ETH > > From: Emma Finn <emma.finn@intel.com> > > This commit includes infrastructure changes for enabling set_masked_X > actions and also adds support for the AVX512 implementation of the > eth_set_addrs action. > > Signed-off-by: Emma Finn <emma.finn@intel.com> > --- > lib/odp-execute-avx512.c | 90 +++++++++++++++++++++++++++++++++++++++ > lib/odp-execute-private.c | 14 ++++++ > lib/odp-execute-private.h | 3 ++ > lib/odp-execute.c | 49 +++++++++++---------- > lib/odp-execute.h | 3 ++ > 5 files changed, 137 insertions(+), 22 deletions(-) > LGTM, Acked-by: Sunil Pai G <sunil.pai.g@intel.com>
> From: Emma Finn <emma.finn@intel.com> > > This commit includes infrastructure changes for enabling set_masked_X > actions and also adds support for the AVX512 implementation of the > eth_set_addrs action. > > Signed-off-by: Emma Finn <emma.finn@intel.com> > --- > lib/odp-execute-avx512.c | 90 +++++++++++++++++++++++++++++++++++++++ > lib/odp-execute-private.c | 14 ++++++ > lib/odp-execute-private.h | 3 ++ > lib/odp-execute.c | 49 +++++++++++---------- > lib/odp-execute.h | 3 ++ > 5 files changed, 137 insertions(+), 22 deletions(-) > > diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c > index 3449acff7..8ecdaecf6 100644 > --- a/lib/odp-execute-avx512.c > +++ b/lib/odp-execute-avx512.c > @@ -23,6 +23,7 @@ > > #include "dp-packet.h" > #include "immintrin.h" > +#include "odp-execute.h" > #include "odp-execute-private.h" > #include "odp-netlink.h" > #include "openvswitch/vlog.h" > @@ -50,6 +51,16 @@ BUILD_ASSERT_DECL(offsetof(struct dp_packet, l3_ofs) + > BUILD_ASSERT_DECL(sizeof(struct dp_packet) - > offsetof(struct dp_packet, l2_pad_size) >= sizeof(__m128i)); > > +/* The below build assert makes sure the order of the fields needed by > + * the set masked functions shuffle operations do not change. This should not > + * happen as these are defined under the Linux uapi. */ > +BUILD_ASSERT_DECL(offsetof(struct ovs_key_ethernet, eth_src) + > + MEMBER_SIZEOF(struct ovs_key_ethernet, eth_src) == > + offsetof(struct ovs_key_ethernet, eth_dst)); > + > +/* Array of callback functions, one for each masked operation. */ > +odp_execute_action_cb impl_set_masked_funcs[__OVS_KEY_ATTR_MAX]; > + > static inline void ALWAYS_INLINE > avx512_dp_packet_resize_l2(struct dp_packet *b, int resize_by_bytes) > { > @@ -207,6 +218,80 @@ action_avx512_push_vlan(struct dp_packet_batch *batch, const struct nlattr *a) > } > } > > +/* This function performs the same operation on each packet in the batch as > + * the scalar odp_eth_set_addrs() function. */ > +static void > +action_avx512_eth_set_addrs(struct dp_packet_batch *batch, > + const struct nlattr *a) > +{ > + const struct ovs_key_ethernet *key, *mask; > + struct dp_packet *packet; > + > + a = nl_attr_get(a); > + key = nl_attr_get(a); > + mask = odp_get_key_mask(a, struct ovs_key_ethernet); > + > + /* Read the content of the key(src) and mask in the respective registers. > + * We only load the src and dest addresses, which is only 96-bits and not > + * 128-bits. */ > + __m128i v_src = _mm_maskz_loadu_epi32(0x7,(void *) key); > + __m128i v_mask = _mm_maskz_loadu_epi32(0x7, (void *) mask); One question here I asked throughout the various revisions but got not answered: "The second load, loads 128 bits of data, but there are only 12 bytes to load. What happens if the memory at the remaining 6 bytes are not mapped in memory (i.e. a page does not exist/can't be loaded)? Will we crash!? Guess the key is fine, as we will read some bytes of the mask data." <SNIP> > +void > +odp_execute_scalar_action(struct dp_packet_batch *batch, > + const struct nlattr *action) > +{ > + enum ovs_action_attr type = nl_attr_type(action); > + > + if (action_impls[ACTION_IMPL_SCALAR].funcs[type] && > + type <= OVS_ACTION_ATTR_MAX) { Guess the two checks above need to be reversed, i.e. the type <= OVS_ACTION_ATTR_MAX should be first. > + > + action_impls[ACTION_IMPL_SCALAR].funcs[type](batch, action); > + } > +} <SNIP> > +static void > +action_set_masked(struct dp_packet_batch *batch, const struct nlattr *a) > +{ > + const struct nlattr *key = nl_attr_get(a); > + struct dp_packet *packet; > + > + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { > + odp_execute_masked_set_action(packet, key); > + } Indentation is off here. <SNIP> The rest of the patch looks good to me. //Eelco
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Thursday, July 14, 2022 2:24 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com> > Cc: dev@openvswitch.org; i.maximets@ovn.org; Amber, Kumar > <kumar.amber@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Finn, Emma > <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com> > Subject: Re: [PATCH v10 09/10] odp-execute: Add ISA implementation of set_masked > ETH <snip patch> > > + /* Read the content of the key(src) and mask in the respective registers. > > + * We only load the src and dest addresses, which is only 96-bits and not > > + * 128-bits. */ > > + __m128i v_src = _mm_maskz_loadu_epi32(0x7,(void *) key); > > + __m128i v_mask = _mm_maskz_loadu_epi32(0x7, (void *) mask); > > One question here I asked throughout the various revisions but got not answered: > > "The second load, loads 128 bits of data, but there are only 12 bytes to load. What > happens if the memory at the remaining 6 bytes are not mapped in memory (i.e. a > page does not exist/can't be loaded)? Will we crash!? AVX512 has some very nice features for handling scenarios where "not full" SIMD is required. This feature is known as "k-masks", and in short allows "turning off" part of the SIMD instruction from having an effect. In this case, the "maskz" part of the intrinsic means that the k-mask becomes active. An extra parameter is added to any k-mask instruction (_mm_maskz_*), which indicates what lanes to enable/disable. Note that the *size* of each lane is determined by the end of the intrinsic, so _epi32() indicates 32-bit lanes. A worked example below: _mm_maskz_loadu_epi32(0x7, (void *) mask); kmask is 0x7, or "111" in binary, so lowest 3 lanes (visualize them on the right) are active. As the instruction targets 32-bit ints, each lane size is 4 bytes, so 3 * 4 = 12 bytes "active". As a result, only 12 bytes are loaded from memory here. Even if the next byte was on a new page, and not mapped into our virtual address range, there would be no crash here due to the k-mask handling the load. <snip more patch>
On 14 Jul 2022, at 16:11, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Thursday, July 14, 2022 2:24 PM >> To: Van Haaren, Harry <harry.van.haaren@intel.com> >> Cc: dev@openvswitch.org; i.maximets@ovn.org; Amber, Kumar >> <kumar.amber@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Finn, Emma >> <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com> >> Subject: Re: [PATCH v10 09/10] odp-execute: Add ISA implementation of set_masked >> ETH > > <snip patch> > >>> + /* Read the content of the key(src) and mask in the respective registers. >>> + * We only load the src and dest addresses, which is only 96-bits and not >>> + * 128-bits. */ >>> + __m128i v_src = _mm_maskz_loadu_epi32(0x7,(void *) key); >>> + __m128i v_mask = _mm_maskz_loadu_epi32(0x7, (void *) mask); >> >> One question here I asked throughout the various revisions but got not answered: >> >> "The second load, loads 128 bits of data, but there are only 12 bytes to load. What >> happens if the memory at the remaining 6 bytes are not mapped in memory (i.e. a >> page does not exist/can't be loaded)? Will we crash!? > > AVX512 has some very nice features for handling scenarios where "not full" SIMD is > required. This feature is known as "k-masks", and in short allows "turning off" part of > the SIMD instruction from having an effect. > > In this case, the "maskz" part of the intrinsic means that the k-mask becomes active. > An extra parameter is added to any k-mask instruction (_mm_maskz_*), which indicates > what lanes to enable/disable. Note that the *size* of each lane is determined by the > end of the intrinsic, so _epi32() indicates 32-bit lanes. A worked example below: > > _mm_maskz_loadu_epi32(0x7, (void *) mask); > > kmask is 0x7, or "111" in binary, so lowest 3 lanes (visualize them on the right) are active. > As the instruction targets 32-bit ints, each lane size is 4 bytes, so 3 * 4 = 12 bytes "active". > As a result, only 12 bytes are loaded from memory here. Even if the next byte was on a new > page, and not mapped into our virtual address range, there would be no crash here due to > the k-mask handling the load. > > <snip more patch> Thanks, that really answers my question! I guess I should better read the pseudo code on the intrinsics guide :) //Eelco
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c index 3449acff7..8ecdaecf6 100644 --- a/lib/odp-execute-avx512.c +++ b/lib/odp-execute-avx512.c @@ -23,6 +23,7 @@ #include "dp-packet.h" #include "immintrin.h" +#include "odp-execute.h" #include "odp-execute-private.h" #include "odp-netlink.h" #include "openvswitch/vlog.h" @@ -50,6 +51,16 @@ BUILD_ASSERT_DECL(offsetof(struct dp_packet, l3_ofs) + BUILD_ASSERT_DECL(sizeof(struct dp_packet) - offsetof(struct dp_packet, l2_pad_size) >= sizeof(__m128i)); +/* The below build assert makes sure the order of the fields needed by + * the set masked functions shuffle operations do not change. This should not + * happen as these are defined under the Linux uapi. */ +BUILD_ASSERT_DECL(offsetof(struct ovs_key_ethernet, eth_src) + + MEMBER_SIZEOF(struct ovs_key_ethernet, eth_src) == + offsetof(struct ovs_key_ethernet, eth_dst)); + +/* Array of callback functions, one for each masked operation. */ +odp_execute_action_cb impl_set_masked_funcs[__OVS_KEY_ATTR_MAX]; + static inline void ALWAYS_INLINE avx512_dp_packet_resize_l2(struct dp_packet *b, int resize_by_bytes) { @@ -207,6 +218,80 @@ action_avx512_push_vlan(struct dp_packet_batch *batch, const struct nlattr *a) } } +/* This function performs the same operation on each packet in the batch as + * the scalar odp_eth_set_addrs() function. */ +static void +action_avx512_eth_set_addrs(struct dp_packet_batch *batch, + const struct nlattr *a) +{ + const struct ovs_key_ethernet *key, *mask; + struct dp_packet *packet; + + a = nl_attr_get(a); + key = nl_attr_get(a); + mask = odp_get_key_mask(a, struct ovs_key_ethernet); + + /* Read the content of the key(src) and mask in the respective registers. + * We only load the src and dest addresses, which is only 96-bits and not + * 128-bits. */ + __m128i v_src = _mm_maskz_loadu_epi32(0x7,(void *) key); + __m128i v_mask = _mm_maskz_loadu_epi32(0x7, (void *) mask); + + + /* These shuffle masks are used below, and each position tells where to + * move the bytes to. So here, the fourth sixth byte in + * ovs_key_ethernet is moved to byte location 0 in v_src/v_mask. + * The seventh is moved to 1, etc., etc. + * This swap is needed to move the src and dest MAC addresses in the + * same order as in the ethernet packet. */ + static const uint8_t eth_shuffle[16] = { + 6, 7, 8, 9, 10, 11, 0, 1, + 2, 3, 4, 5, 0xFF, 0xFF, 0xFF, 0xFF + }; + + /* Load the shuffle mask in v_shuf. */ + __m128i v_shuf = _mm_loadu_si128((void *) eth_shuffle); + + /* Swap the key/mask src and dest addresses to the ethernet order. */ + v_src = _mm_shuffle_epi8(v_src, v_shuf); + v_mask = _mm_shuffle_epi8(v_mask, v_shuf); + + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { + + struct eth_header *eh = dp_packet_eth(packet); + + if (!eh) { + continue; + } + + /* Load the first 128-bits of the packet into the v_ether register. */ + __m128i v_dst = _mm_loadu_si128((void *) eh); + + /* AND the v_mask to the packet data (v_dst). */ + __m128i dst_masked = _mm_andnot_si128(v_mask, v_dst); + + /* OR the new addresses (v_src) with the masked packet addresses + * (dst_masked). */ + __m128i res = _mm_or_si128(v_src, dst_masked); + + /* Write back the modified ethernet addresses. */ + _mm_storeu_si128((void *) eh, res); + } +} + +static void +action_avx512_set_masked(struct dp_packet_batch *batch, const struct nlattr *a) +{ + const struct nlattr *mask = nl_attr_get(a); + enum ovs_key_attr attr_type = nl_attr_type(mask); + + if (attr_type <= OVS_KEY_ATTR_MAX && impl_set_masked_funcs[attr_type]) { + impl_set_masked_funcs[attr_type](batch, a); + } else { + odp_execute_scalar_action(batch, a); + } +} + int action_avx512_init(struct odp_execute_action_impl *self OVS_UNUSED) { @@ -214,6 +299,11 @@ action_avx512_init(struct odp_execute_action_impl *self OVS_UNUSED) * are identified by OVS_ACTION_ATTR_*. */ self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_avx512_pop_vlan; self->funcs[OVS_ACTION_ATTR_PUSH_VLAN] = action_avx512_push_vlan; + self->funcs[OVS_ACTION_ATTR_SET_MASKED] = action_avx512_set_masked; + + /* Set function pointers for the individual operations supported by the + * SET_MASKED action. */ + impl_set_masked_funcs[OVS_KEY_ATTR_ETHERNET] = action_avx512_eth_set_addrs; return 0; } diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c index 2fabf6c62..ec42d3d17 100644 --- a/lib/odp-execute-private.c +++ b/lib/odp-execute-private.c @@ -22,6 +22,7 @@ #include "cpu.h" #include "dpdk.h" #include "dp-packet.h" +#include "odp-execute.h" #include "odp-execute-private.h" #include "odp-netlink.h" #include "odp-util.h" @@ -242,6 +243,19 @@ action_autoval_generic(struct dp_packet_batch *batch, const struct nlattr *a) dp_packet_delete_batch(&original_batch, true); } +void +odp_execute_scalar_action(struct dp_packet_batch *batch, + const struct nlattr *action) +{ + enum ovs_action_attr type = nl_attr_type(action); + + if (action_impls[ACTION_IMPL_SCALAR].funcs[type] && + type <= OVS_ACTION_ATTR_MAX) { + + action_impls[ACTION_IMPL_SCALAR].funcs[type](batch, action); + } +} + int action_autoval_init(struct odp_execute_action_impl *self) { diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h index f66e6e6d1..b3707783f 100644 --- a/lib/odp-execute-private.h +++ b/lib/odp-execute-private.h @@ -100,4 +100,7 @@ int action_avx512_init(struct odp_execute_action_impl *self); void odp_execute_action_get_info(struct ds *name); +void odp_execute_scalar_action(struct dp_packet_batch *batch, + const struct nlattr *action); + #endif /* ODP_EXTRACT_PRIVATE */ diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 0c5837640..dafb198bb 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -562,8 +562,6 @@ odp_execute_set_action(struct dp_packet *packet, const struct nlattr *a) } } -#define get_mask(a, type) ((const type *)(const void *)(a + 1) + 1) - static void odp_execute_masked_set_action(struct dp_packet *packet, const struct nlattr *a) @@ -575,17 +573,17 @@ odp_execute_masked_set_action(struct dp_packet *packet, switch (type) { case OVS_KEY_ATTR_PRIORITY: md->skb_priority = nl_attr_get_u32(a) - | (md->skb_priority & ~*get_mask(a, uint32_t)); + | (md->skb_priority & ~*odp_get_key_mask(a, uint32_t)); break; case OVS_KEY_ATTR_SKB_MARK: md->pkt_mark = nl_attr_get_u32(a) - | (md->pkt_mark & ~*get_mask(a, uint32_t)); + | (md->pkt_mark & ~*odp_get_key_mask(a, uint32_t)); break; case OVS_KEY_ATTR_ETHERNET: odp_eth_set_addrs(packet, nl_attr_get(a), - get_mask(a, struct ovs_key_ethernet)); + odp_get_key_mask(a, struct ovs_key_ethernet)); break; case OVS_KEY_ATTR_NSH: { @@ -595,27 +593,27 @@ odp_execute_masked_set_action(struct dp_packet *packet, case OVS_KEY_ATTR_IPV4: odp_set_ipv4(packet, nl_attr_get(a), - get_mask(a, struct ovs_key_ipv4)); + odp_get_key_mask(a, struct ovs_key_ipv4)); break; case OVS_KEY_ATTR_IPV6: odp_set_ipv6(packet, nl_attr_get(a), - get_mask(a, struct ovs_key_ipv6)); + odp_get_key_mask(a, struct ovs_key_ipv6)); break; case OVS_KEY_ATTR_TCP: odp_set_tcp(packet, nl_attr_get(a), - get_mask(a, struct ovs_key_tcp)); + odp_get_key_mask(a, struct ovs_key_tcp)); break; case OVS_KEY_ATTR_UDP: odp_set_udp(packet, nl_attr_get(a), - get_mask(a, struct ovs_key_udp)); + odp_get_key_mask(a, struct ovs_key_udp)); break; case OVS_KEY_ATTR_SCTP: odp_set_sctp(packet, nl_attr_get(a), - get_mask(a, struct ovs_key_sctp)); + odp_get_key_mask(a, struct ovs_key_sctp)); break; case OVS_KEY_ATTR_MPLS: @@ -623,33 +621,33 @@ odp_execute_masked_set_action(struct dp_packet *packet, if (mh) { put_16aligned_be32(&mh->mpls_lse, nl_attr_get_be32(a) | (get_16aligned_be32(&mh->mpls_lse) - & ~*get_mask(a, ovs_be32))); + & ~*odp_get_key_mask(a, ovs_be32))); } break; case OVS_KEY_ATTR_ARP: set_arp(packet, nl_attr_get(a), - get_mask(a, struct ovs_key_arp)); + odp_get_key_mask(a, struct ovs_key_arp)); break; case OVS_KEY_ATTR_ND: odp_set_nd(packet, nl_attr_get(a), - get_mask(a, struct ovs_key_nd)); + odp_get_key_mask(a, struct ovs_key_nd)); break; case OVS_KEY_ATTR_ND_EXTENSIONS: odp_set_nd_ext(packet, nl_attr_get(a), - get_mask(a, struct ovs_key_nd_extensions)); + odp_get_key_mask(a, struct ovs_key_nd_extensions)); break; case OVS_KEY_ATTR_DP_HASH: md->dp_hash = nl_attr_get_u32(a) - | (md->dp_hash & ~*get_mask(a, uint32_t)); + | (md->dp_hash & ~*odp_get_key_mask(a, uint32_t)); break; case OVS_KEY_ATTR_RECIRC_ID: md->recirc_id = nl_attr_get_u32(a) - | (md->recirc_id & ~*get_mask(a, uint32_t)); + | (md->recirc_id & ~*odp_get_key_mask(a, uint32_t)); break; case OVS_KEY_ATTR_TUNNEL: /* Masked data not supported for tunnel. */ @@ -857,6 +855,17 @@ action_push_vlan(struct dp_packet_batch *batch, const struct nlattr *a) } } +static void +action_set_masked(struct dp_packet_batch *batch, const struct nlattr *a) +{ + const struct nlattr *key = nl_attr_get(a); + struct dp_packet *packet; + + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { + odp_execute_masked_set_action(packet, key); + } +} + /* Implementation of the scalar actions impl init function. Build up the * array of func ptrs here. */ @@ -867,6 +876,7 @@ odp_action_scalar_init(struct odp_execute_action_impl *self) * are identified by OVS_ACTION_ATTR_*. */ self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_pop_vlan; self->funcs[OVS_ACTION_ATTR_PUSH_VLAN] = action_push_vlan; + self->funcs[OVS_ACTION_ATTR_SET_MASKED] = action_set_masked; return 0; } @@ -1078,12 +1088,6 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, } break; - case OVS_ACTION_ATTR_SET_MASKED: - DP_PACKET_BATCH_FOR_EACH(i, packet, batch) { - odp_execute_masked_set_action(packet, nl_attr_get(a)); - } - break; - case OVS_ACTION_ATTR_SAMPLE: DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { odp_execute_sample(dp, packet, steal && last_action, a, @@ -1210,6 +1214,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, /* The following actions are handled by the scalar implementation. */ case OVS_ACTION_ATTR_POP_VLAN: case OVS_ACTION_ATTR_PUSH_VLAN: + case OVS_ACTION_ATTR_SET_MASKED: OVS_NOT_REACHED(); } diff --git a/lib/odp-execute.h b/lib/odp-execute.h index 0921ee924..2ba1ec5d2 100644 --- a/lib/odp-execute.h +++ b/lib/odp-execute.h @@ -46,4 +46,7 @@ void odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, const struct nlattr *actions, size_t actions_len, odp_execute_cb dp_execute_action); + +#define odp_get_key_mask(a, type) ((const type *)(const void *)(a + 1) + 1) + #endif