On Sun, Jun 22, 2025 at 12:52:45AM +0300, Vitaliy Makkoveev wrote:
> On Sat, Jun 21, 2025 at 11:30:32PM +0200, Alexander Bluhm wrote:
> > On Sat, Jun 21, 2025 at 10:21:02PM +0300, Vitaliy Makkoveev wrote:
> > > They both are the same and both do `rtgeneration' update. As I
> > > understand the code, we don't need to serialize `ipmultipath' and
> > > `rtgeneration' update with the netstack, however, want to increment
> > > `rtgeneration' before `ipmultipath' update. IIRC, atomic operations will
> > > not be reordered and we don't need any extra barriers here.
> > 
> > This is architecture dependent.  membar_enter_after_atomic() is not
> > defined everywhere.  I think membar_consumer() is needed here.  See
> > comments inline.
> > 
> > bluhm
> > 
> >     atomic_store_int(&ipmultipath, newval);
> >     membar_producer();
> >     atomic_inc_long(&rtgeneration);
> > 
> > This ensures that route cache gets invalid after ipmultipath changes.
> > You need that as rt_hash() result is based on ipmultipath.  That
> > way the ipmultipath loads in rt_hash() and route_cache() can be
> > kept independent.  No lock needed.
> 
> Could we have the side effects while two threads set `ipmultipath' to
> opposing values? I mean
> 
> 	/* ipmultipath is 1 */
> 	atomic_store_int(&ipmultipath, 0); /* thread A */
> 	atomic_store_int(&ipmultipath, 1); /* thread B */
> 	atomic_inc_long(&rtgeneration); /* thread B */
> 	atomic_inc_long(&rtgeneration); /* thread A */
> 
> If so, I like to keep patch serialized.

As usual with concurrent sysctl, last write wins.  There is no point
in serializing interger writes of userland threads.  Important is
consistent kernel view.  And in your example atomic_inc_long(&rtgeneration)
reloads the routing cache at next use.  It does not matter which
thread does it.  It just has to happen after writing ipmultipath
on the current CPU.

bluhm