| Message ID | 167481813172.1146345.9238649094707887654.stgit@ebuild.local |
|---|---|
| State | Accepted |
| Commit | e1e5eac5b0167c65c802bd60ed37605b1e1c9c92 |
| Headers | show |
| Series | [ovs-dev,v2] tc: Add TCA_KIND flower to delete and get operation to avoid rtnl_lock(). | 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 |
On 27/01/2023 13:16, Eelco Chaudron wrote: > A long long time ago, an effort was made to make tc flower > rtnl_lock() free. However, on the OVS part we forgot to add > the TCA_KIND "flower" attribute, which tell the kernel to skip > the lock. This patch corrects this by adding the attribute for > the delete and get operations. > > The kernel code calls tcf_proto_is_unlocked() to determine the > rtnl_lock() is needed for the specific tc protocol. It does this > in the tc_new_tfilter(), tc_del_tfilter(), and in tc_get_tfilter(). > > If the name is not set, tcf_proto_is_unlocked() will always return > false. If set, the specific protocol is queried for unlocked support. > > Fixes: f98e418fbdb6 ("tc: Add tc flower functions") > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/netdev-linux.c | 2 +- > lib/netdev-offload-tc.c | 20 ++++++++++---------- > lib/tc.c | 10 +++++++++- > lib/tc.h | 3 ++- > 4 files changed, 22 insertions(+), 13 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index f6d7a1b97..65bdd51db 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2735,7 +2735,7 @@ tc_del_matchall_policer(struct netdev *netdev) > } > > id = tc_make_tcf_id(ifindex, block_id, prio, TC_INGRESS); > - err = tc_del_filter(&id); > + err = tc_del_filter(&id, "matchall"); > if (err) { > return err; > } > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index ce7f8ad97..2d7e74b1a 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -239,7 +239,7 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid) > { > int err; > > - err = tc_del_filter(id); > + err = tc_del_flower_filter(id); > if (!err) { > del_ufid_tc_mapping(ufid); > } > @@ -461,7 +461,7 @@ delete_chains_from_netdev(struct netdev *netdev, struct tcf_id *id) > */ > HMAP_FOR_EACH_POP (chain_node, node, &map) { > id->chain = chain_node->chain; > - tc_del_filter(id); > + tc_del_flower_filter(id); > free(chain_node); > } > } > @@ -482,7 +482,7 @@ netdev_tc_flow_flush(struct netdev *netdev) > continue; > } > > - err = tc_del_filter(&data->id); > + err = tc_del_flower_filter(&data->id); > if (!err) { > del_ufid_tc_mapping_unlocked(&data->ufid); > } > @@ -2498,13 +2498,13 @@ probe_multi_mask_per_prio(int ifindex) > > id2 = tc_make_tcf_id(ifindex, block_id, prio, TC_INGRESS); > error = tc_replace_flower(&id2, &flower); > - tc_del_filter(&id1); > + tc_del_flower_filter(&id1); > > if (error) { > goto out; > } > > - tc_del_filter(&id2); > + tc_del_flower_filter(&id2); > > multi_mask_per_prio = true; > VLOG_INFO("probe tc: multiple masks on single tc prio is supported."); > @@ -2556,7 +2556,7 @@ probe_ct_state_support(int ifindex) > goto out_del; > } > > - tc_del_filter(&id); > + tc_del_flower_filter(&id); > ct_state_support = OVS_CS_F_NEW | > OVS_CS_F_ESTABLISHED | > OVS_CS_F_TRACKED | > @@ -2570,7 +2570,7 @@ probe_ct_state_support(int ifindex) > goto out_del; > } > > - tc_del_filter(&id); > + tc_del_flower_filter(&id); > > /* Test for ct_state INVALID support */ > memset(&flower, 0, sizeof flower); > @@ -2581,7 +2581,7 @@ probe_ct_state_support(int ifindex) > goto out; > } > > - tc_del_filter(&id); > + tc_del_flower_filter(&id); > ct_state_support |= OVS_CS_F_INVALID; > > /* Test for ct_state REPLY support */ > @@ -2597,7 +2597,7 @@ probe_ct_state_support(int ifindex) > ct_state_support |= OVS_CS_F_REPLY_DIR; > > out_del: > - tc_del_filter(&id); > + tc_del_flower_filter(&id); > out: > tc_add_del_qdisc(ifindex, false, 0, TC_INGRESS); > VLOG_INFO("probe tc: supported ovs ct_state bits: 0x%x", ct_state_support); > @@ -2750,7 +2750,7 @@ netdev_tc_init_flow_api(struct netdev *netdev) > > /* fallback here if delete chains fail */ > if (!get_chain_supported) { > - tc_del_filter(&id); > + tc_del_flower_filter(&id); > } > > /* make sure there is no ingress/egress qdisc */ > diff --git a/lib/tc.c b/lib/tc.c > index 447ab376e..1fb2b4a92 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -2337,14 +2337,21 @@ parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]) > } > > int > -tc_del_filter(struct tcf_id *id) > +tc_del_filter(struct tcf_id *id, const char *kind) > { > struct ofpbuf request; > > request_from_tcf_id(id, 0, RTM_DELTFILTER, NLM_F_ACK, &request); > + nl_msg_put_string(&request, TCA_KIND, kind); > return tc_transact(&request, NULL); > } > > +int > +tc_del_flower_filter(struct tcf_id *id) > +{ > + return tc_del_filter(id, "flower"); > +} > + > int > tc_get_flower(struct tcf_id *id, struct tc_flower *flower) > { > @@ -2353,6 +2360,7 @@ tc_get_flower(struct tcf_id *id, struct tc_flower *flower) > int error; > > request_from_tcf_id(id, 0, RTM_GETTFILTER, NLM_F_ECHO, &request); > + nl_msg_put_string(&request, TCA_KIND, "flower"); > error = tc_transact(&request, &reply); > if (error) { > return error; > diff --git a/lib/tc.h b/lib/tc.h > index a828fd3e3..ea4ce806b 100644 > --- a/lib/tc.h > +++ b/lib/tc.h > @@ -384,7 +384,8 @@ struct tc_flower { > }; > > int tc_replace_flower(struct tcf_id *id, struct tc_flower *flower); > -int tc_del_filter(struct tcf_id *id); > +int tc_del_filter(struct tcf_id *id, const char *kind); > +int tc_del_flower_filter(struct tcf_id *id); > int tc_get_flower(struct tcf_id *id, struct tc_flower *flower); > int tc_dump_flower_start(struct tcf_id *id, struct nl_dump *dump, bool terse); > int tc_dump_tc_chain_start(struct tcf_id *id, struct nl_dump *dump); > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev thanks. looks good. Did few ovs tc tests to verify all good. Reviewed-by: Roi Dayan <roid@nvidia.com>
On 1/29/23 09:18, Roi Dayan wrote: > > > On 27/01/2023 13:16, Eelco Chaudron wrote: >> A long long time ago, an effort was made to make tc flower >> rtnl_lock() free. However, on the OVS part we forgot to add >> the TCA_KIND "flower" attribute, which tell the kernel to skip >> the lock. This patch corrects this by adding the attribute for >> the delete and get operations. >> >> The kernel code calls tcf_proto_is_unlocked() to determine the >> rtnl_lock() is needed for the specific tc protocol. It does this >> in the tc_new_tfilter(), tc_del_tfilter(), and in tc_get_tfilter(). >> >> If the name is not set, tcf_proto_is_unlocked() will always return >> false. If set, the specific protocol is queried for unlocked support. >> >> Fixes: f98e418fbdb6 ("tc: Add tc flower functions") >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> --- > thanks. looks good. > Did few ovs tc tests to verify all good. > > Reviewed-by: Roi Dayan <roid@nvidia.com> Thanks! Applied and backported down to 2.17. Best regards, Ilya Maximets.
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index f6d7a1b97..65bdd51db 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2735,7 +2735,7 @@ tc_del_matchall_policer(struct netdev *netdev) } id = tc_make_tcf_id(ifindex, block_id, prio, TC_INGRESS); - err = tc_del_filter(&id); + err = tc_del_filter(&id, "matchall"); if (err) { return err; } diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index ce7f8ad97..2d7e74b1a 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -239,7 +239,7 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid) { int err; - err = tc_del_filter(id); + err = tc_del_flower_filter(id); if (!err) { del_ufid_tc_mapping(ufid); } @@ -461,7 +461,7 @@ delete_chains_from_netdev(struct netdev *netdev, struct tcf_id *id) */ HMAP_FOR_EACH_POP (chain_node, node, &map) { id->chain = chain_node->chain; - tc_del_filter(id); + tc_del_flower_filter(id); free(chain_node); } } @@ -482,7 +482,7 @@ netdev_tc_flow_flush(struct netdev *netdev) continue; } - err = tc_del_filter(&data->id); + err = tc_del_flower_filter(&data->id); if (!err) { del_ufid_tc_mapping_unlocked(&data->ufid); } @@ -2498,13 +2498,13 @@ probe_multi_mask_per_prio(int ifindex) id2 = tc_make_tcf_id(ifindex, block_id, prio, TC_INGRESS); error = tc_replace_flower(&id2, &flower); - tc_del_filter(&id1); + tc_del_flower_filter(&id1); if (error) { goto out; } - tc_del_filter(&id2); + tc_del_flower_filter(&id2); multi_mask_per_prio = true; VLOG_INFO("probe tc: multiple masks on single tc prio is supported."); @@ -2556,7 +2556,7 @@ probe_ct_state_support(int ifindex) goto out_del; } - tc_del_filter(&id); + tc_del_flower_filter(&id); ct_state_support = OVS_CS_F_NEW | OVS_CS_F_ESTABLISHED | OVS_CS_F_TRACKED | @@ -2570,7 +2570,7 @@ probe_ct_state_support(int ifindex) goto out_del; } - tc_del_filter(&id); + tc_del_flower_filter(&id); /* Test for ct_state INVALID support */ memset(&flower, 0, sizeof flower); @@ -2581,7 +2581,7 @@ probe_ct_state_support(int ifindex) goto out; } - tc_del_filter(&id); + tc_del_flower_filter(&id); ct_state_support |= OVS_CS_F_INVALID; /* Test for ct_state REPLY support */ @@ -2597,7 +2597,7 @@ probe_ct_state_support(int ifindex) ct_state_support |= OVS_CS_F_REPLY_DIR; out_del: - tc_del_filter(&id); + tc_del_flower_filter(&id); out: tc_add_del_qdisc(ifindex, false, 0, TC_INGRESS); VLOG_INFO("probe tc: supported ovs ct_state bits: 0x%x", ct_state_support); @@ -2750,7 +2750,7 @@ netdev_tc_init_flow_api(struct netdev *netdev) /* fallback here if delete chains fail */ if (!get_chain_supported) { - tc_del_filter(&id); + tc_del_flower_filter(&id); } /* make sure there is no ingress/egress qdisc */ diff --git a/lib/tc.c b/lib/tc.c index 447ab376e..1fb2b4a92 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -2337,14 +2337,21 @@ parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]) } int -tc_del_filter(struct tcf_id *id) +tc_del_filter(struct tcf_id *id, const char *kind) { struct ofpbuf request; request_from_tcf_id(id, 0, RTM_DELTFILTER, NLM_F_ACK, &request); + nl_msg_put_string(&request, TCA_KIND, kind); return tc_transact(&request, NULL); } +int +tc_del_flower_filter(struct tcf_id *id) +{ + return tc_del_filter(id, "flower"); +} + int tc_get_flower(struct tcf_id *id, struct tc_flower *flower) { @@ -2353,6 +2360,7 @@ tc_get_flower(struct tcf_id *id, struct tc_flower *flower) int error; request_from_tcf_id(id, 0, RTM_GETTFILTER, NLM_F_ECHO, &request); + nl_msg_put_string(&request, TCA_KIND, "flower"); error = tc_transact(&request, &reply); if (error) { return error; diff --git a/lib/tc.h b/lib/tc.h index a828fd3e3..ea4ce806b 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -384,7 +384,8 @@ struct tc_flower { }; int tc_replace_flower(struct tcf_id *id, struct tc_flower *flower); -int tc_del_filter(struct tcf_id *id); +int tc_del_filter(struct tcf_id *id, const char *kind); +int tc_del_flower_filter(struct tcf_id *id); int tc_get_flower(struct tcf_id *id, struct tc_flower *flower); int tc_dump_flower_start(struct tcf_id *id, struct nl_dump *dump, bool terse); int tc_dump_tc_chain_start(struct tcf_id *id, struct nl_dump *dump);
A long long time ago, an effort was made to make tc flower rtnl_lock() free. However, on the OVS part we forgot to add the TCA_KIND "flower" attribute, which tell the kernel to skip the lock. This patch corrects this by adding the attribute for the delete and get operations. The kernel code calls tcf_proto_is_unlocked() to determine the rtnl_lock() is needed for the specific tc protocol. It does this in the tc_new_tfilter(), tc_del_tfilter(), and in tc_get_tfilter(). If the name is not set, tcf_proto_is_unlocked() will always return false. If set, the specific protocol is queried for unlocked support. Fixes: f98e418fbdb6 ("tc: Add tc flower functions") Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/netdev-linux.c | 2 +- lib/netdev-offload-tc.c | 20 ++++++++++---------- lib/tc.c | 10 +++++++++- lib/tc.h | 3 ++- 4 files changed, 22 insertions(+), 13 deletions(-)