+Cc Roi
On Thu 26 Jan 2023 at 04:33, Marcelo Ricardo Leitner <mleit...@redhat.com>
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.
>
> 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.
>
Thanks for the explanation Marcelo. And thank you Eelco for fixing this!
>> >
>> > Signed-off-by: Eelco Chaudron <echau...@redhat.com>
>> > ---
>> > 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;
>> > }
>> >
>> > +
>> > int
>> > -tc_del_filter(struct tcf_id *id)
>> > +tc_del_filter(struct tcf_id *id, 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 +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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
