On 2020-08-04 2:44 AM, Ilya Maximets wrote:
> On 8/3/20 12:58 PM, Roi Dayan wrote:
>> There is no need to pass tc rules to hw when just probing
>> for tc features. this will avoid redundant errors from hw drivers
>> that may happen.
>
> That makes sense. Thanks!
> Few comments inline.
>
>>
>> Signed-off-by: Roi Dayan <r...@mellanox.com>
>> ---
>> lib/netdev-offload-tc.c | 2 ++
>> lib/tc.c | 13 ++++++-------
>> lib/tc.h | 8 ++++++++
>> 3 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index 2c9c6f4cae8b..18ff380f9861 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -1918,6 +1918,7 @@ probe_multi_mask_per_prio(int ifindex)
>>
>> memset(&flower, 0, sizeof flower);
>>
>> + flower.tc_policy = TC_POLICY_SKIP_HW;
>> flower.key.eth_type = htons(ETH_P_IP);
>> flower.mask.eth_type = OVS_BE16_MAX;
>> memset(&flower.key.dst_mac, 0x11, sizeof flower.key.dst_mac);
>> @@ -1965,6 +1966,7 @@ probe_tc_block_support(int ifindex)
>>
>> memset(&flower, 0, sizeof flower);
>>
>> + flower.tc_policy = TC_POLICY_SKIP_HW;
>> flower.key.eth_type = htons(ETH_P_IP);
>> flower.mask.eth_type = OVS_BE16_MAX;
>> memset(&flower.key.dst_mac, 0x11, sizeof flower.key.dst_mac);
>> diff --git a/lib/tc.c b/lib/tc.c
>> index c96d095381d7..8761304c92bb 100644
>> --- a/lib/tc.c
>> +++ b/lib/tc.c
>> @@ -65,12 +65,6 @@ VLOG_DEFINE_THIS_MODULE(tc);
>>
>> static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(60, 5);
>>
>> -enum tc_offload_policy {
>> - TC_POLICY_NONE,
>> - TC_POLICY_SKIP_SW,
>> - TC_POLICY_SKIP_HW
>> -};
>> -
>> static enum tc_offload_policy tc_policy = TC_POLICY_NONE;
>>
>> struct tc_pedit_key_ex {
>> @@ -2757,6 +2751,7 @@ nl_msg_put_flower_options(struct ofpbuf *request,
>> struct tc_flower *flower)
>> bool is_vlan = eth_type_vlan(flower->key.eth_type);
>> bool is_qinq = is_vlan && eth_type_vlan(flower->key.encap_eth_type[0]);
>> bool is_mpls = eth_type_mpls(flower->key.eth_type);
>> + enum tc_offload_policy policy = flower->tc_policy;
>> int err;
>>
>> /* need to parse acts first as some acts require changing the matching
>> @@ -2882,7 +2877,11 @@ nl_msg_put_flower_options(struct ofpbuf *request,
>> struct tc_flower *flower)
>> }
>> }
>>
>> - nl_msg_put_u32(request, TCA_FLOWER_FLAGS,
>> tc_get_tc_cls_policy(tc_policy));
>> + if (policy == TC_POLICY_NONE) {
>> + policy = tc_policy;
>> + }
>> +
>> + nl_msg_put_u32(request, TCA_FLOWER_FLAGS, tc_get_tc_cls_policy(policy));
>>
>> if (flower->tunnel) {
>> nl_msg_put_flower_tunnel(request, flower);
>> diff --git a/lib/tc.h b/lib/tc.h
>> index 028eed5d0658..78f433ceef77 100644
>> --- a/lib/tc.h
>> +++ b/lib/tc.h
>> @@ -312,6 +312,12 @@ is_tcf_id_eq(struct tcf_id *id1, struct tcf_id *id2)
>> && id1->chain == id2->chain;
>> }
>>
>> +enum tc_offload_policy {
>> + TC_POLICY_NONE,
>
> Since now the code actually depends on this value to be zero, it might be
> good to explicitly set it, or have a build assertion. This will protect us
> from possible issues if someone will try to modify this enum later.
> i.e.
> TC_POLICY_NONE = 0,
>
great. I did both.
>> + TC_POLICY_SKIP_SW,
>> + TC_POLICY_SKIP_HW
>> +};
>> +
>> struct tc_flower {
>> struct tc_flower_key key;
>> struct tc_flower_key mask;
>> @@ -337,6 +343,8 @@ struct tc_flower {
>> bool needs_full_ip_proto_mask;
>>
>> enum tc_offloaded_state offloaded_state;
>> + /* used to force skip_hw when probing tc features */
>
> I see that there is a comment below in this file that doesn't follow OVS
> coding
> style, but, please, use the OVS-style comments for a new code, i.e. start with
> a capital letter and end with a period.
>
fixed. sent v2.
>> + enum tc_offload_policy tc_policy;
>> };
>>
>> /* assert that if we overflow with a masked write of uint32_t to the last
>> byte
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
