Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

Jakub Kicinski
2022-10-01T02:03:40Z

On Thu, 29 Sep 2022 11:35:05 +0800 Hangbin Liu wrote:
> In commit 81c7288b170a ("sched: cls: enable verbose logging") Marcelo
> made cls could log verbose info for offloading failures, which helps
> improving Open vSwitch debuggability when using flower offloading.
>
> It would also be helpful if "tc monitor" could log this message, as it
> doesn't require vswitchd log level adjusment. Let's add the extack message
> in tfilter_notify so the monitor program could receive the failures.
> e.g.

The title read as "just another extack addition" but this is much
more than that :S

Jamal, you may want to take a look.

> # tc monitor
> added chain dev enp3s0f1np1 parent ffff: chain 0
> added filter dev enp3s0f1np1 ingress protocol all pref 49152 flower chain 0 handle 0x1
> ct_state +trk+new
> not_in_hw
> action order 1: gact action drop
> random type none pass val 0
> index 1 ref 1 bind 1
>
> Warning: mlx5_core: matching on ct_state +new isn't supported.
>
> Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>
> Rebase the patch to latest net-next as the previous could not
> apply to net-next.

> + nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm),
> + (extack && extack->_msg) ? flags | NLM_F_MULTI : flags);

> +
> + if (extack && extack->_msg) {
> + nlh = nlmsg_put(skb, portid, seq, NLMSG_DONE, 0, flags | NLM_F_ACK_TLVS);
> + if (!nlh)
> + goto out_nlmsg_trim;
> +
> + if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
> + goto nla_put_failure;
> +
> + nlmsg_end(skb, nlh);
> + }
> +

So you're adding a fake* _F_MULTI on the notification just so you
can queue a NLMSG_DONE after and not break the "NLMSG_DONE terminates
a _F_MUTLI" sequence rule?

* fake as in there's only one message, there's no multi-ness here.

I don't think _F_MULTI should be treated lightly and I don't think
NLMSG_DONE as part of notification sequences is a good idea either :(

(1) does the tracepoint not give you want you need?
(netlink:netlink_extack), failing that -
(2) why not wrap the extack msg in an attribute