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 <[email protected]>
> ---
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
thanks. looks good.
Did few ovs tc tests to verify all good.
Reviewed-by: Roi Dayan <[email protected]>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
