[ovs-dev] [PATCH] ofproto: Recycle least recently used ofport.

Gurucharan Shetty shettyg at nicira.com
Wed Sep 25 17:50:13 UTC 2013


On Tue, Sep 24, 2013 at 10:52 AM, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Aug 26, 2013 at 10:25:39AM -0700, Gurucharan Shetty wrote:
>> If there is a lot of churn in creation and deletion of
>> interfaces, we may end up recycling the ofport value of a
>> recently deleted interface for a newly created interface.
>> This may result in an old stale openflow rule applying
>> on the newly created interface.
>>
>> With this commit, when a new port is added, try and provide
>> it an ofport value that has not been used during the current
>> run. If such value does not exist, provide the least
>> recently used ofport value.
>>
>> Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
>
> Have I really been somehow ignoring this patch for a month?  Sorry about
> that.
No problem. There were more urgent things going on in the last month.
>
> I wonder whether we should consider sufficiently old entries (an hour, a
> day) to be expired.
I had thought about it. Consider a case where someone is testing OVS with
adding and deleting interfaces continuously. Initially, he would not have any
 problems adding his interfaces, but as the test continues, the number of
interfaces that can be created decreases and I felt that can be confusing.

With the current implementation, since we have ~65,000 ofport options,
it feels unlikely that
we can actually use a very recently deleted interface soon in a real
world use case.

>
> It looks like struct ofport_usage and its related functions are used
> only in ofproto.c, so it would be nice to move the struct definition in
> there and make the functions 'static'.

Will do,

>
>> @@ -1748,35 +1750,45 @@ alloc_ofp_port(struct ofproto *ofproto, const char *netdev_name)
>>      port_idx = port_idx ? port_idx : UINT16_MAX;
>>
>>      if (port_idx >= ofproto->max_ports
>> -        || bitmap_is_set(ofproto->ofp_port_ids, port_idx)) {
>> -        uint16_t end_port_no = ofproto->alloc_port_no;
>
> There's an extra pair of parentheses here:
Corrected.

Thanks.
Guru

>
>> +        || (ofport_get_usage(ofproto, u16_to_ofp(port_idx)) == LLONG_MAX)) {
>> +        uint16_t lru_ofport = 0, end_port_no = ofproto->alloc_port_no;
>> +        long long int last_used_at, lru = LLONG_MAX;
>
> Acked-by: Ben Pfaff <blp at nicira.com>



More information about the dev mailing list