Re: [PATCH net] fib_rules: return 0 directly if an exactly same rule exists when NLM_F_EXCL not supplied

From: "Maciej Żenczykowski" <zenczykowski@gmail.com>
To: David Ahern <dsahern@gmail.com>
Cc: Lorenzo Colitti <lorenzo@google.com>,
	David Ahern <dsa@cumulusnetworks.com>,
	Hangbin Liu <liuhangbin@gmail.com>,
	David Miller <davem@davemloft.net>, Yaro Slav <yaro330@gmail.com>,
	Thomas Haller <thaller@redhat.com>,
	Alistair Strachan <astrachan@google.com>,
	Greg KH <greg@kroah.com>, Linux NetDev <netdev@vger.kernel.org>,
	Mateusz Bajorski <mateusz.bajorski@nokia.com>
Subject: Re: [PATCH net] fib_rules: return 0 directly if an exactly same rule exists when NLM_F_EXCL not supplied
Date: Thu, 6 Jun 2019 16:01:34 -0700	[thread overview]
Message-ID: <CAHo-OowgWNicSEmvatfk-D6Xwo-gGphCt34CLxaPOHu4VE6Law@mail.gmail.com> (raw)
In-Reply-To: <d8022629-0359-34b7-ccae-bb12b190e43b@gmail.com>

(side note: change was reverted in net stable)

On Wed, Jun 5, 2019 at 8:33 AM David Ahern <dsahern@gmail.com> wrote:
> On 6/4/19 10:58 PM, Lorenzo Colitti wrote:
> > As for making this change in 5.3: we might be able to structure the
> > code differently in a future Android release, assuming the same
> > userspace code can work on kernels back to 4.4 (not sure it can, since
> > the semantics changed in 4.8). But even if we can fix this in Android,
> > this change is still breaking compatibility with existing other
> > userspace code. Are there concrete performance optimizations that
> > you'd like to make that can't be made unless you change the semantics
> > here? Are those optimizations worth breaking the backwards
> > compatibility guarantees for?
>
> The list of fib rules is walked looking for a match. more rules = more
> overhead. Given the flexibility of the rules, I have not come up with
> any changes that have a real improvement in that overhead. VRF, which
> uses policy routing, was changed to have a single l3mdev rule for all
> VRFs for this reason.

Instead of keeping duplicates there could be a counter on the singleton rule.
And then adding/removing is just inc/dec on the counter (and only
actually delete when counter drops to 0).
Would require some extra effort to make it look the same when dumping
I guess (to expand it out).

---

I'm not sure this is worth optimizing for.  In Android these states
with dupes are temporary.
And really, if you complain about performance it is perfectly
reasonable to say "then don't create duplicate rules".

---

Note though that from a multithreaded perspective, you'd never want
the 'ignore it and return 0' behaviour.
It's fundamentally a bad api.  It's better to return an error and
userspace can decide that EEXIST is acceptable and treat it as 0.

Imagine two threads doing 'add ip rule foo; blah blah; remove ip rule foo'

You either want the create dupes, or the 'return EEXIST' behaviour (so
the second thread can fail, or wait until it succeeds or whatever).

This way if two threads both run the same operation, either both of
them succeed,
or one succeeds and the other gets notified of dup.

Otherwise you get a spurious failure in one of the threads and bad
behaviour in the other (since rule vanishes before it should).