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

[Robert Shearman <rshearma@brocade.com>]

On 28/11/15 06:15, roopa wrote:
> 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.

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