Re: [PATCH net-next v2 00/12] net: sched: propagate extack to cls offloads on destroy and only with skip_sw

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Thu, Jan 25, 2018 at 02:57:17PM -0800, Jakub Kicinski wrote:
> On Thu, 25 Jan 2018 13:11:57 -0200, Marcelo Ricardo Leitner wrote:
> > On Wed, Jan 24, 2018 at 12:54:12PM -0800, Jakub Kicinski wrote:
> > > Hi!
> > > 
> > > This series some of Jiri's comments and the fact that today drivers
> > > may produce extack even if there is no skip_sw flag (meaning the
> > > driver failure is not really a problem), and warning messages will
> > > only confuse the users.  
> > 
> > It's a fair point, but I think this is not the best solution. How will
> > the user then know why it failed to install in hw? Will have to
> > install a new rule, just with skip_sw, and hope that it fails with the
> > same reason?
> >
> > Maybe it's better to let tc/ovs/etc only exhibit this information
> > under a certain log/debug level.
> 
> What you say does make sense in case of classifiers which are basically
> HW offload vehicles.  But for cls_bpf, which people are actually using
> heavily as a software solution, I don't want any warning to be produced
> just because someone happened to run the command on a Netronome
> card :(  Say someone swaps an old NIC for a NFP, and runs their old
> cls_bpf scripts and suddenly there are warnings they don't care about
> and have no way of silencing.

(Sorry for the delay on replying, btw. I'm still traveling.)

Makes sense. I agree that at least it shouldn't be displayed in a way
that may lead the user to think it was a big/fatal error.

> 
> I do think skip_sw will fail for the same reason, unless someone adds
> extacks for IO or memory allocation problems or other transient things.

I don't really follow this one. Fail you mean, fail to report the
actual reason? If so, ok, but that's something easily fixable I think,
especially because with skip_sw, if such an error happens, it's fatal
for the operation so the error reporting is consistent.

> 
> Do I understand correctly that OvS TC does not set skip_sw?  We could

Yes.

> add a "verbose offload" flag to the API or filter the bad extacks at
> the user space level.  Although, again, my preference would be not to
> filter at the user space level, because user space can't differentiate
> between a probably-doesn't-matter-but-HW-offload-failed warning or legit
> something-is-not-right-in-the-software-but-command-succeeded warning :S

But userspace is the original requester. It should know what the rule
is intended for and how to act upon it. For ovs, for example, it could
just log a message and move on, while tc could report "hey, ok, but
please note that the rule couldn't be offloaded".

> So if there is a major use for non-forced offload failure warnings I
> would lean towards a new flag.

I'm thinking about this, still undecided. In the end maybe a counter
somewhere could be enough and such reporting is not needed. Thinking..

  Marcelo