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

Jamal Hadi Salim
2022-11-02T15:33:40Z

On Wed, Oct 26, 2022 at 5:58 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Sun, Oct 02, 2022 at 11:27:08AM -0400, Jamal Hadi Salim wrote:
> > > 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]

> > 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?).

> >
> > 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