[PATCH] Free interfaces in shutdown state

[Lorenz Brun]

Hi Maria

I’m running this with ASan/UBSan for some time, so at least for my usecases this hasn’t generated any memory unsafety.
But as far as I read your post this would only break if I explicitly configured interfaces which aren’t available during initial startup (or reconfiguration), which I don’t so I guess this is expected.

The problem I’m trying to solve is that I run containers on hosts running Bird which can eventually create and delete an unbounded number of network interfaces, making Bird consume more and more CPU up to an entire core at which point it starts slowing down and eventually becomes too slow to properly participate in routing protocols. So basically it’d be fine if I leak interfaces stated explicitly in the configuration (these would be bounded, in most cases to <100), but not any interface showing up on the host (unbounded, 100K+).

Regards,
Lorenz

> On 13 Jul 2022, at 09:47, Maria Matejka <maria.matejka at nic.cz> wrote:
> 
> Hello!
> 
> I tried to implement this myself several times, yet this simple approach doesn't work. The configuration system also creates "pending" interfaces if something depends on a certain interface name. This patch would delete these "pending" interfaces, rendering some pointers invalid.
> 
> The right way to do this is to introduce some kind of reference counting to interface objects, as e.g. the cached route attribute objects do. Something like this is probably going to land in some future version of BIRD 3, anyway if you decided to implement this for version 2 as well, I won't complain.
> 
> Thank you for your contribution!
> 
> Maria
> 
> On 7/12/22 7:54 PM, Lorenz Brun wrote:
>> When interfaces are created and destroyed by automated systems,
>> a potentially unbounded amount of interfaces exist.
>> This results in bird consuming ever-increasing amounts of memory and
>> CPU time for the list of known interfaces.
>> This change frees the interface after it is in shutdown state and all
>> notifications for it have been processed.
>> ---
>>  nest/iface.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>> diff --git a/nest/iface.c b/nest/iface.c
>> index 682340c5..b4c65512 100644
>> --- a/nest/iface.c
>> +++ b/nest/iface.c
>> @@ -324,7 +324,7 @@ if_update(struct iface *new)
>>  	if_copy(i, new);
>>  	if (c)
>>  	  if_notify_change(c, i);
>> -
>> +
>>  	i->flags |= IF_UPDATED;
>>  	return i;
>>        }
>> @@ -365,10 +365,10 @@ if_end_partial_update(struct iface *i)
>>  void
>>  if_end_update(void)
>>  {
>> -  struct iface *i;
>> +  struct iface *i, *x;
>>    struct ifa *a, *b;
>>  -  WALK_LIST(i, iface_list)
>> +  WALK_LIST_DELSAFE(i, x, iface_list)
>>      {
>>        if (!(i->flags & IF_UPDATED))
>>  	if_change_flags(i, (i->flags & ~IF_ADMIN_UP) | IF_SHUTDOWN);
>> @@ -379,6 +379,11 @@ if_end_update(void)
>>  	      ifa_delete(a);
>>  	  if_end_partial_update(i);
>>  	}
>> +    if ((i->flags & IF_SHUTDOWN)) {
>> +      WALK_LIST_DELSAFE(a, b, i->addrs)
>> +        ifa_delete(a);
>> +      rem_node(&i->n);
>> +    }
>>      }
>>  }
>>  
>