Re: [PATCH] ipv6: don't call addrconf_dst_alloc again when enable lo

[chenweilong <chenweilong@huawei.com>]

On 2014/1/8 16:55, Hannes Frederic Sowa wrote:
> On Wed, Jan 08, 2014 at 04:42:46PM +0800, Gao feng wrote:
>> On 01/08/2014 04:05 PM, Hannes Frederic Sowa wrote:
>>> On Wed, Jan 08, 2014 at 03:50:09PM +0800, Gao feng wrote:
>>>> On 01/03/2014 02:53 PM, Hannes Frederic Sowa wrote:
>>>>> On Thu, Jan 02, 2014 at 05:33:15PM +0800, chenweilong wrote:
>>>>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>>>>>> index 62d1799..d2f8c0a 100644
>>>>>> --- a/net/ipv6/addrconf.c
>>>>>> +++ b/net/ipv6/addrconf.c
>>>>>> @@ -2422,8 +2422,9 @@ static void init_loopback(struct net_device *dev)
>>>>>>  			if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE))
>>>>>>  				continue;
>>>>>>
>>>>>> -			if (sp_ifa->rt)
>>>>>> -				continue;
>>>>>> +			if (sp_ifa->rt && sp_ifa->rt->dst.dev == dev) {
>>>>>> +				ip6_del_rt(sp_ifa->rt);
>>>>>> +			}
>>>>>>
>>>>>>  			sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, 0);
>>>>>>
>>>>>
>>>>> Maybe this change would not be that bad after all, as those ifa attached dsts
>>>>> are already dead and queued up for gc and should not get inserted back.
>>>>
>>>> I like this idea, maybe the below patch is better. we only need to delete this
>>>> route when it has been added to garbage list.
>>>>
>>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>>>> index 1a341f7..4dca886 100644
>>>> --- a/net/ipv6/addrconf.c
>>>> +++ b/net/ipv6/addrconf.c
>>>> @@ -2610,8 +2610,16 @@ static void init_loopback(struct net_device *dev)
>>>>                         if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE))
>>>>                                 continue;
>>>>
>>>> -                       if (sp_ifa->rt)
>>>> -                               continue;
>>>> +                       if (sp_ifa->rt) {
>>>> +                               /* This dst has been added to garbage list when
>>>> +                                * lo device down, delete this obsolete dst and
>>>> +                                * reallocate new router for ifa. */
>>>> +                               if (sp_ifa->rt->dst.obsolete > 0) {
>>>> +                                       ip6_del_rt(sp_ifa->rt);
>>>> +                                       sp_ifa->rt = NULL;
>>>> +                               } else
>>>> +                                       continue;
>>>> +                       }
>>>>
>>>>                         sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, false);
>>>
>>> It looks like it can work but I don't know if we should just fix this the
>>> clean way (see below).
>>>
>>>>> I'll try to just disable routes without removing them at all when we set an
>>>>> interface to down at the weekend.
>>>>>
>>>>
>>>> How do you decide which route should be disabled?  use rt6_flags? I don't know
>>>> if your way will cause miscarriage.
>>>
>>> What I did so far is that I added a new function next to rt6_ifdown that
>>> only gets called if interface gets shutdown but not unregistered (from
>>> addrconf_ifdown).
>>>
>>
>> rt6_ifdown has alreay put this device related routes to the garbage list.
>>
>>> fib6_clean_all then iterates over the whole routing table with a new predicate
>>> function which checks in the same way like fib6_ifdown, if it is a matching route
>>> (the interfaces match up) and if so, toggles a new "DEAD" flag in rt6i_flags.
>>>
>>> When bringing up the interface I distinguish between up and register and just
>>> clear this death flag from the routes on bringing it up.
>>>
>>> fib lookup code then does not honour those routes.
>>>
>>> I had no time to test this thoroughly at the weekend and still have some code
>>> paths were I am unsure. Do you see any problems with this so far? We could
>>> then delete the special cases on loopback interface init.
>>
>> So you add a special case in the general network interface down logic. I think this
>> is more complex...
> 
> The problem is, that we only have a reference to ifp->rt, the loopback
> RTF_LOCAL route. Currently we silently remove all other routes this interface
> has. Sure, they come back soon with RAs and such, but this is not the way this
> should work.
> 
> Greetings,
> 
>   Hannes
> 
> 
> .
> 

Hi,

It's quite a long time, How's your patch going on?