On Wed, Oct 26, 2022 at 5:58 AM Hangbin Liu [off-list ref] wrote:

On Sun, Oct 02, 2022 at 11:27:08AM -0400, Jamal Hadi Salim wrote:

quoted

quoted

But, precisely. In the example Hangbin gave, it is showing why the
entry is not_in_hw. That's still data that belongs to the event that
happened and that can't be queried afterwards even if the user/app
monitoring it want to. Had it failed entirely, I agree, as the control
path never changed.

tc monitor is easier to use than perf probes in some systems. It's not
uncommon to have tc installed but not perf. It's also easier to ask a
customer to run it than explain how to enable the tracepoint and print
ftrace buffer via /sys files, and the output is more meaningful for us
as well: we know exactly which filter triggered the message. The only
other place that we can correlate the filter and the warning, is on
vswitchd log. Which is not easy to read either.

To Jakub's point: I think one of those NLMSGERR TLVs is the right place
and while traces look attractive I see the value of having a unified
collection point via the tc monitor.

Sorry for the latency - was at conference and still in travel mode...

Hi Jamal,

Sorry for the late response. I just came back form vacation. For this issue,
I saw netlink_dump_done() also put NLMSGERR_ATTR_MSG in NLMSG_DONE.
So why can't we do the same here?

In https://www.kernel.org/doc/html//next/userspace-api/netlink/intro.html,
The "optionally extended ACK" in NLMSG_DONE is OK.

Ok.
[That seemd to  be a nice doc - need to find time to look at it]

quoted

IMO:
I think if you need to do this, then you have to teach iproute2
new ways of interpreting the message (which is nice because you
dont have to worry about backward compat). Some of that code
should be centralized and reused by netlink generically
instead of just cls_api, example the whole NLM_F_ACK_TLVS dance.

Would you please help explain more about this?

I meant you only added it for filter notification - but such a feature would
be useful also for other tc pieces (like actions and qdiscs). Is there a better
way to do it such that the other tc parts may benefit (instead of just
filter_notify?).

quoted

Also - i guess it will depend on the underlying driver?
This seems very related to a specific driver:
"Warning: mlx5_core: matching on ct_state +new isn't supported."
Debuggability is always great but so is backwards compat.
What happens when you run old userspace tc? There are tons
of punting systems that process these events out there and
depend on the current event messages as is.

I think old tc should just ignore this NLMSGERR_ATTR_MSG?

Yes.
So looks good to me then.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal