On 27 Jan 2023, at 11:36, Ilya Maximets wrote:
> 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?
Will add this in my own words to not violate GPL license ;)
> Also, should we consider this as a bug fix? If so, the Fixes tag
> might be useful.
Yes will add a fixed tag as I think this is a bug fix.
> 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.
Oops, will fix in v2
>>>> 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.
ACK, will fix in v2
>>>> {
>>>> 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
