From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "Jesper Dangaard Brouer" <brouer@redhat.com>,
"Stephen Hemminger" <stephen@networkplumber.org>,
netdev@vger.kernel.org,
"Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
"Toke Høiland-Jørgensen" <toke@redhat.com>,
ruxandra.radulescu@nxp.com, ioana.ciornei@nxp.com,
nipun.gupta@nxp.com, shawnguo@kernel.org
Subject: Re: [PATCH net-next 2/2] dpaa2-eth: fix return codes used in ndo_setup_tc
Date: Mon, 27 Apr 2020 11:18:43 -0300 [thread overview]
Message-ID: <20200427141843.GB2469@localhost.localdomain> (raw)
In-Reply-To: <20200424172859.78245218@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Fri, Apr 24, 2020 at 05:28:59PM -0700, Jakub Kicinski wrote:
> On Fri, 24 Apr 2020 09:04:26 +0200 Jesper Dangaard Brouer wrote:
> > (Side-note: First I placed an extack in qdisc_create_dflt() but I
> > realized it was wrong, because it could potentially override messages
> > from the lower layers.)
> >
> > > > but doing that would require a change
> > > > to the ndo_setup_tc hook to allow driver to return its own error message
> > > > as to why the setup failed.
> > >
> > > Yeah :S The block offload command contains extack, but this driver
> > > doesn't understand block offload, so it won't interpret it...
> > >
> > > That brings me to an important point - doesn't the extack in patch 1
> > > override any extack driver may have set?
> >
> > Nope, see above side-note. I set the extack at the "lowest level",
> > e.g. closest to the error that cause the err back-propagation, when I
> > detect that this will cause a failure at higher level.
>
> Still, the driver is lower:
>
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 2908e0a0d6e1..ffed75453c14 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -209,9 +209,12 @@ static int
> nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
> {
> struct netdevsim *ns = netdev_priv(dev);
> + struct flow_block_offload *f;
>
> switch (type) {
> case TC_SETUP_BLOCK:
> + f = type_data;
> + NL_SET_ERR_MSG_MOD(f->extack, "bla bla bla bla bla");
> + return -EINVAL;
> - return flow_block_cb_setup_simple(type_data,
> - &nsim_block_cb_list,
> - nsim_setup_tc_block_cb,
> default:
> return -EOPNOTSUPP;
> }
>
> # tc qdisc add dev netdevsim0 ingress
> Error: Driver ndo_setup_tc failed.
>
>
> > > I remember we discussed this when adding extacks to the TC core, but
> > > I don't remember the conclusion now, ugh.
> >
> > When adding the extack code, I as puzzled that during debugging I
> > managed to override other extack messages. Have anyone though about a
> > better way to handle if extack messages gets overridden?
>
> I think there was more discussion, but this is all I can find now:
>
> https://lore.kernel.org/netdev/20180918131212.20266-4-johannes@sipsolutions.net/#t
>
> Maybe Marcelo will remeber.
There was also this other one, on supporting multiple messages:
https://lore.kernel.org/netdev/673abaddb26351826ca454f46d1271f1f4814c56.1521226621.git.marcelo.leitner%40gmail.com/T/
What I remember is what we have now is enough because of 3 main
reasons:
- Anything logged before the actual error is potentially just noise
- Anything logged after the actual error is just noise,
- The user is probably not prepared to handle warnings from ip/tc/etc
commands.
For this last, one example using the context here:
"Warning: failed to create the qdisc."
Ok, but what does that mean? Should the sysadmin, potentially unaware
of what a qdisc is, retry or ignore the warning? If retry, then it
probably should have just failed itself in the first place.
next prev parent reply other threads:[~2020-04-27 14:18 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-23 14:57 [PATCH net-next 0/2] Fix qdisc noop issue caused by driver and identify future bugs Jesper Dangaard Brouer
2020-04-23 14:57 ` [PATCH net-next 1/2] net: sched: report ndo_setup_tc failures via extack Jesper Dangaard Brouer
2020-04-23 19:51 ` Ioana Ciornei
2020-04-23 14:57 ` [PATCH net-next 2/2] dpaa2-eth: fix return codes used in ndo_setup_tc Jesper Dangaard Brouer
2020-04-23 15:28 ` Stephen Hemminger
2020-04-23 15:38 ` Jesper Dangaard Brouer
2020-04-23 19:33 ` Stephen Hemminger
2020-04-23 19:56 ` Jakub Kicinski
2020-04-24 7:04 ` Jesper Dangaard Brouer
2020-04-25 0:28 ` Jakub Kicinski
2020-04-27 14:18 ` Marcelo Ricardo Leitner [this message]
2020-04-23 19:49 ` Ioana Ciornei
2020-04-24 23:45 ` [PATCH net-next 0/2] Fix qdisc noop issue caused by driver and identify future bugs David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200427141843.GB2469@localhost.localdomain \
--to=marcelo.leitner@gmail.com \
--cc=brouer@redhat.com \
--cc=ilias.apalodimas@linaro.org \
--cc=ioana.ciornei@nxp.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nipun.gupta@nxp.com \
--cc=ruxandra.radulescu@nxp.com \
--cc=shawnguo@kernel.org \
--cc=stephen@networkplumber.org \
--cc=toke@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).