>-----Original Message-----
>From: Stokes, Ian <ian.sto...@intel.com>
>Sent: Friday, August 14, 2020 4:03 PM
>To: Ilya Maximets <i.maxim...@ovn.org>; Eli Britstein <el...@nvidia.com>; Finn,
>Emma <emma.f...@intel.com>; d...@openvswitch.org
>Cc: Xing, Beilei <beilei.x...@intel.com>
>Subject: RE: [PATCH] netdev-offload-dpdk: Fix for broken ethernet matching
>HWOL for XL710 NIC
>
>
>> On 8/14/20 2:29 PM, Stokes, Ian wrote:
>> >> On 8/14/20 10:58 AM, Stokes, Ian wrote:
>> >>>>> -----Original Message-----
>> >>>>> From: Ilya Maximets <i.maxim...@ovn.org>
>> >>>>> Sent: Thursday, August 13, 2020 9:26 PM
>> >>>>> To: Emma Finn <emma.f...@intel.com>; d...@openvswitch.org
>> >>>>> Cc: ian.sto...@intel.com; i.maxim...@ovn.org; Eli Britstein
>> >>>>> <el...@nvidia.com>; beilei.x...@intel.com; i.maxim...@ovn.org
>> >>>>> 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 <emma.f...@intel.com>
>> >>>>>> Signed-off-by: Eli Britstein <el...@nvidia.com>
>> >>>>>> Co-authored-by: Eli Britstein <el...@nvidia.com>
>> >>>>>> ---
>> >>>>>> 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)
>> >
>> > Just a quick one here Ilya, sparse throws an restricted ovs_be16
>> > degrades to integer error with the above line, are you ok with the
>> > following change
>> >
>> > if (match->wc.masks.dl_type == htons(0xFFFF) &&
>> > is_ip_any(&match->flow)
>>
>> Sure. I'd prefer keep the lowercase for a constant, but that is not
>> really important.
>
>No problem, I'll change it to lower before sending the v3, just waiting on
>Travis
>now but looks ok.
OVS_BE16_MAX
>
>Regards
>Ian
>>
>> >
>> > BR
>> > Ian
>> >> && 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.
>> >>
>> >>>
>> >>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
