Message-ID: <CAM0EoMkBZx0qk5MR4jyeFn5T7eRco2n_Y58hryDtLTBjaOzEfg@mail.gmail.com>
Date: Tue, 29 Oct 2024 09:48:34 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>, 
	Pedro Tammela <pctammela@...atatu.com>, Victor Nogueira <victor@...atatu.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net/sched: sch_api: fix xa_insert() error path in tcf_block_get_ext()

On Fri, Oct 25, 2024 at 8:40 AM Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> On Thu, Oct 24, 2024 at 11:39:51AM -0400, Jamal Hadi Salim wrote:
> > On Wed, Oct 23, 2024 at 6:05 AM Vladimir Oltean <vladimir.oltean@....com> wrote:
> > >
> > > This command:
> > >
> > > $ tc qdisc replace dev eth0 ingress_block 1 egress_block 1 clsact
> > > Error: block dev insert failed: -EBUSY.
> > >
> > > fails because user space requests the same block index to be set for
> > > both ingress and egress.
> > >
> > > [ side note, I don't think it even failed prior to commit 913b47d3424e
> > >   ("net/sched: Introduce tc block netdev tracking infra"), because this
> > >   is a command from an old set of notes of mine which used to work, but
> > >   alas, I did not scientifically bisect this ]
> > >
> >
> > What would be the use case for having both share the same index?
> > Mirror action for example could be used to target a group of ports
> > grouped by blockid in which case a unique blockid simplifies.
> >
> > > The problem is not that it fails
>
> As mentioned, I don't have a use case for sharing block indices between
> ingress and egress. I did have old commands which used to not fail
> (incorrectly, one might say), but they stopped working without notice,
> and the kernel was not being very obvious about it. Had the kernel
> behavior in this case been more clear/consistent, and not failed any
> subsequent command I would type in, even if valid, it would have taken
> me less time to find out. Hence this patch, and also another one I have
> prepared for net-next which improves the error message.
>
> > Fix makes  sense.
> > Acked-by: Jamal Hadi Salim <jhs@...atatu.com>
>
> Thanks.
>
> > I am also hoping you did run the tdc tests (despite this not looking
> > like it breaks any existing feature)
>
> To be honest, I had not, because I had doubts that this error path would
> be exercised in any of the tests (and I still don't think it is).
>
> But I did run them now, they seem to pass, except for the last 11 of
> them which seem to be skipped, and I really do not have the patience
> right now to debug and see why.
>

You did fine, more below...

> ~/selftests/tc-testing# ./tdc.py
>  -- scapy/SubPlugin.__init__
>  -- ns/SubPlugin.__init__

[..]
>
> ok 1159 4cbd - Try to add filter with duplicate key # skipped - Tests using the DEV2 variable must define the name of a physical NIC with the -d option when running tdc.
> Test has been skipped.
>
> ok 1160 7c65 - Add flower filter and then terse dump it # skipped - Tests using the DEV2 variable must define the name of a physical NIC with the -d option when running tdc.
> Test has been skipped.
>
> ok 1161 d45e - Add flower filter and verify that terse dump doesn't output filter key # skipped - Tests using the DEV2 variable must define the name of a physical NIC with the -d option when
> running tdc.
> Test has been skipped.

These were skipped because it would require real hardware (typically
also for testing offload).

cheers,
jamal

Powered by blists - more mailing lists