| Message ID | 20200803105806.215737-1-roid@mellanox.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] tc: Use skip_hw flag when probing tc features | expand |
On Mon, Aug 03, 2020 at 01:58:06PM +0300, 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. > > Signed-off-by: Roi Dayan <roid@mellanox.com> Thanks, this looks fine to me. I'll let it sit for a day or so to see if anyone has any comments.
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 <roid@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, > + 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. > + enum tc_offload_policy tc_policy; > }; > > /* assert that if we overflow with a masked write of uint32_t to the last byte >
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 <roid@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 >> >
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, + 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 */ + enum tc_offload_policy tc_policy; }; /* assert that if we overflow with a masked write of uint32_t to the last byte
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. Signed-off-by: Roi Dayan <roid@mellanox.com> --- lib/netdev-offload-tc.c | 2 ++ lib/tc.c | 13 ++++++------- lib/tc.h | 8 ++++++++ 3 files changed, 16 insertions(+), 7 deletions(-)