On 28/11/15 06:15, roopa wrote:

On 11/25/15, 8:18 AM, David Miller wrote:

quoted

From: Roopa Prabhu <redacted>
Date: Tue, 24 Nov 2015 15:22:22 -0800

quoted

v4 -v5
	- if kmemdup fails, modify the original route in place. This is a
	corner case and only side effect is that in the remote case
	of kmemdup failure, the changes will not be atomically visible
	to datapath.

I really don't like this.

Either you need to make the changes appear atomic to the data path,
and you therefore must fail the operation if kmemdup() fails, or it
doesn't matter and you should just always change the route in-place.

As far as I can tell it can't possibly matter.  The alive counter is
never modified by the data path, it is only tested to make a multipath
decision.  Likewise it's rather harmless to send a frame or two via a
device currently going down.

But if you're convinced it matters, then is matters, and you can't
fake things when kmemdup() fails.  And in that case I would recommend
that you use a two pass algorithm, one pass allocates all of the
new routes, and the second fills them in, inserts them, and frees
the old ones.

That is the only way you can unwind and fail cleanly.

And oh yeah, that's right, you can't really fail this and make the
ifdown not proceed.

So you're stuck, right?

That's why this has to be designed in a way where memory allocations
are not necessary.  These notifiers aren't really designed to facilitate
situations that require resource acquisitions that can fail.

Get your point. I am not convinced that the atomic update matters much for the transient case.
  I was trying to accommodate comments i got during the review and it seemed ok to cover both
cases by optionally doing the atomic update when we can. But, I get your position on this.

My comment on atomic updates on v2 was referring to whether partial 
updates could be seen by the forwarding code and if they did whether it 
matters. I don't think it matters for nh_flags, but I think it does for 
rt_nhn_alive.

Thanks,
Rob