> On 8/14/20 10:58 AM, Stokes, Ian wrote:
> >>> -----Original Message-----
> >>> From: Ilya Maximets <[email protected]>
> >>> Sent: Thursday, August 13, 2020 9:26 PM
> >>> To: Emma Finn <[email protected]>; [email protected]
> >>> Cc: [email protected]; [email protected]; Eli Britstein
> >>> <[email protected]>; [email protected]; [email protected]
> >>> Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet
> >>> matching HWOL for XL710 NIC
> >>>
> >>>
> >>> On 8/13/20 6:13 PM, Emma Finn wrote:
> >>>> This patch introduces a temporary work around to fix partial
> >>>> hardware offload for XL710 devices. Currently the incorrect
> >>>> ethernet pattern is being set. This patch will be removed once this
> >>>> issue is fixed within the i40e PMD.
> >>>>
> >>>> Signed-off-by: Emma Finn <[email protected]>
> >>>> Signed-off-by: Eli Britstein <[email protected]>
> >>>> Co-authored-by: Eli Britstein <[email protected]>
> >>>> ---
> >>>> lib/netdev-offload-dpdk.c | 35 +++++++++++++++++++++++++----------
> >>>> 1 file changed, 25 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> >>>> index de6101e..ede2077 100644
> >>>> --- a/lib/netdev-offload-dpdk.c
> >>>> +++ b/lib/netdev-offload-dpdk.c
> >>>> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions *actions)
> >>>> actions->cnt = 0;
> >>>> }
> >>>>
> >>>> +/*
> >>>> +* This is a temporary work around to fix ethernet pattern for
> >>>> +partial hardware
> >>>> +* offload for X710 devices. This fix will be reverted once the
> >>>> +issue is fixed
> >>>> +* within the i40e PMD driver.
> >>>> +*/
> >>>> +#define NULL_ETH_MASK_IF_ZEROS \
> >>>> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof eth_mask->src)
> && \
> >>>> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \
> >>>> + patterns->items[0].mask = NULL; \
> >>>> + free(eth_mask); \
> >>>> + }
> >>>
> >>> Could you please address my comments from this e-mail:
> >>>
> >>>
> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.
> >>> op
> >>> envswitch.org%2Fpipermail%2Fovs-dev%2F2020-
> >>>
> August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f
> >>>
> a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a%
> >>>
> 7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y
> >>> m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0
> >>> ?
> >>>
> >>> i.e. convert this macro definition into function.
> >> My previous patch was a POC. Here is a more "nicer" one. No
> >> macro/function, and no redundant allocation/free:
> >> Need to test and obviously finalize.
> >
> > Thanks for this Eli, just a heads up Emma is out of office today but I can
> > test
> this and submit the v2 if you prefer?
> >
> > @Ilya, what are your thoughts on this approach below?
>
> It looks fine, but I'd re-write the if statement in a following way:
>
> if (match->wc.masks.dl_type == 0xffff && is_ip_any(&match->flow)
> && eth_addr_is_zero(match->wc.masks.dl_dst)
> && eth_addr_is_zero(match->wc.masks.dl_src)) {
>
> i.e. lightweight operations first, special functions for the flow type and
> eth_addr
> checking.
>
Sure, just testing this now, looks good so far, going to run through the usual
quick validation and will send the v3 soon.
Regards
Ian
> >
> > BR
> > Ian
> >>
> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> >> index
> >> de6101e4d..c6d293af3 100644
> >> --- a/lib/netdev-offload-dpdk.c
> >> +++ b/lib/netdev-offload-dpdk.c
> >> @@ -696,16 +696,26 @@ parse_flow_match(struct flow_patterns *patterns,
> >> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >> struct rte_flow_item_eth *spec, *mask;
> >>
> >> - spec = xzalloc(sizeof *spec);
> >> - mask = xzalloc(sizeof *mask);
> >> + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */
> >> + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) &&
> >> + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) &&
> >> + match->wc.masks.dl_type == 0xFFFF &&
> >> + (match->flow.dl_type == htons(ETH_TYPE_IP) ||
> >> + match->flow.dl_type == htons(ETH_TYPE_IPV6))) {
> >> + spec = NULL;
> >> + mask = NULL;
> >> + } else {
> >> + spec = xzalloc(sizeof *spec);
> >> + mask = xzalloc(sizeof *mask);
> >>
> >> - memcpy(&spec->dst, &match->flow.dl_dst, sizeof spec->dst);
> >> - memcpy(&spec->src, &match->flow.dl_src, sizeof spec->src);
> >> - spec->type = match->flow.dl_type;
> >> + memcpy(&spec->dst, &match->flow.dl_dst, sizeof spec->dst);
> >> + memcpy(&spec->src, &match->flow.dl_src, sizeof spec->src);
> >> + spec->type = match->flow.dl_type;
> >>
> >> - memcpy(&mask->dst, &match->wc.masks.dl_dst, sizeof mask->dst);
> >> - memcpy(&mask->src, &match->wc.masks.dl_src, sizeof mask->src);
> >> - mask->type = match->wc.masks.dl_type;
> >> + memcpy(&mask->dst, &match->wc.masks.dl_dst, sizeof mask->dst);
> >> + memcpy(&mask->src, &match->wc.masks.dl_src, sizeof mask->src);
> >> + mask->type = match->wc.masks.dl_type;
> >> + }
> >>
> >> memset(&consumed_masks->dl_dst, 0, sizeof consumed_masks-
> >dl_dst);
> >> memset(&consumed_masks->dl_src, 0, sizeof
> >> consumed_masks->dl_src);
> >>>
> >>>> +
> >>>> static int
> >>>> parse_flow_match(struct flow_patterns *patterns,
> >>>> struct match *match) {
> >>>> + struct rte_flow_item_eth *eth_mask = NULL;
> >>>> + struct rte_flow_item_eth *eth_spec = NULL;
> >>>> uint8_t *next_proto_mask = NULL;
> >>>> struct flow *consumed_masks;
> >>>> uint8_t proto = 0;
> >>>> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns
> *patterns,
> >>>> if (match->wc.masks.dl_type ||
> >>>> !eth_addr_is_zero(match->wc.masks.dl_src) ||
> >>>> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >>>> - struct rte_flow_item_eth *spec, *mask;
> >>>>
> >>>> - spec = xzalloc(sizeof *spec);
> >>>> - mask = xzalloc(sizeof *mask);
> >>>> + eth_spec = xzalloc(sizeof *eth_spec);
> >>>> + eth_mask = xzalloc(sizeof *eth_mask);
> >>>>
> >>>> - memcpy(&spec->dst, &match->flow.dl_dst, sizeof spec->dst);
> >>>> - memcpy(&spec->src, &match->flow.dl_src, sizeof spec->src);
> >>>> - spec->type = match->flow.dl_type;
> >>>> + memcpy(ð_spec->dst, &match->flow.dl_dst, sizeof eth_spec-
> >dst);
> >>>> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof eth_spec-
> >src);
> >>>> + eth_spec->type = match->flow.dl_type;
> >>>>
> >>>> - memcpy(&mask->dst, &match->wc.masks.dl_dst, sizeof mask->dst);
> >>>> - memcpy(&mask->src, &match->wc.masks.dl_src, sizeof mask->src);
> >>>> - mask->type = match->wc.masks.dl_type;
> >>>> + memcpy(ð_mask->dst, &match->wc.masks.dl_dst, sizeof
> >>>> + eth_mask-
> >>>> dst);
> >>>> + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof
> >>>> + eth_mask-
> >>>> src);
> >>>> + eth_mask->type = match->wc.masks.dl_type;
> >>>>
> >>>> memset(&consumed_masks->dl_dst, 0, sizeof consumed_masks-
> >>> dl_dst);
> >>>> memset(&consumed_masks->dl_src, 0, sizeof consumed_masks-
> >>> dl_src);
> >>>> consumed_masks->dl_type = 0;
> >>>>
> >>>> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec,
> mask);
> >>>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH,
> >>>> + eth_spec, eth_mask);
> >>>> }
> >>>>
> >>>> /* VLAN */
> >>>> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns *patterns,
> >>>> /* IP v4 */
> >>>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> >>>> struct rte_flow_item_ipv4 *spec, *mask;
> >>>
> >>> Empty line needed.
> >>>
> >>>> + NULL_ETH_MASK_IF_ZEROS;
> >>>>
> >>>> spec = xzalloc(sizeof *spec);
> >>>> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@
> >>>> parse_flow_match(struct flow_patterns *patterns,
> >>>> /* IP v6 */
> >>>> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) {
> >>>> struct rte_flow_item_ipv6 *spec, *mask;
> >>>
> >>> ditto.
> >>>
> >>>> + NULL_ETH_MASK_IF_ZEROS;
> >>>>
> >>>> spec = xzalloc(sizeof *spec);
> >>>> mask = xzalloc(sizeof *mask);
> >>>>
> >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
