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

[Jakub Kicinski <kuba@kernel.org>]

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