Re: [PATCH net-next v5] mpls: support for dead routes

[roopa <roopa@cumulusnetworks.com>]

On 11/25/15, 8:18 AM, David Miller wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> Date: Tue, 24 Nov 2015 15:22:22 -0800
>
>> 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.