On 1/26/23 13:33, Marcelo Ricardo Leitner wrote:
> On Thu, Jan 26, 2023 at 04:23:39AM -0800, Marcelo Ricardo Leitner wrote:
>> +Cc Chris, Paul and Vlad.
>>
>> On Thu, Jan 26, 2023 at 11:08:02AM +0100, 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.
Hi, Eelco. Thanks for the fix!
>
> In case one is wondering how, it is because in kernel it will:
>
> tc_del_tfilter()
> if (!prio ||
> (q && !(q->ops->cl_ops->flags & QDISC_CLASS_OPS_DOIT_UNLOCKED)) ||
> !tcf_proto_is_unlocked(name)) { <---
> rtnl_held = true;
> rtnl_lock();
>
> And:
> tcf_proto_is_unlocked()
> {
> const struct tcf_proto_ops *ops;
> bool ret;
>
> if (strlen(kind) == 0) <---
> return false;
>
> It can't check, so assumes the safest option. Same happens for
> tc_get_tfilter() and tc_add_tfilter(), but for add ovs was already
> adding TCA_KIND.
Might be good to add some of this information to the commit message.
At least describe the kernel behavior with words. What do you think?
Also, should we consider this as a bug fix? If so, the Fixes tag
might be useful.
Some comments inline.
Best regards, Ilya Maximets.
>
>>>
>>> Signed-off-by: Eelco Chaudron <[email protected]>
>>> ---
>>> lib/netdev-linux.c | 2 +-
>>> lib/netdev-offload-tc.c | 20 ++++++++++----------
>>> lib/tc.c | 11 ++++++++++-
>>> lib/tc.h | 3 ++-
>>> 4 files changed, 23 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..64916eb1f 100644
>>> --- a/lib/tc.c
>>> +++ b/lib/tc.c
>>> @@ -2336,15 +2336,23 @@ parse_netlink_to_tc_policer(struct ofpbuf *reply,
>>> uint32_t police_idx[])
>>> return 0;
>>> }
>>>
>>> +
Unnecessary line.
>>> int
>>> -tc_del_filter(struct tcf_id *id)
>>> +tc_del_filter(struct tcf_id *id, char *kind)
It should probably be a const char *, otherwise it may cause some
compiler warnings, in theory, because you're passing static constant
strings into this function.
>>> {
>>> 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 +2361,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..4ecc562d9 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, 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
