[ovs-dev] [PATCH 3/3] [bug 2462] datapath: Increase maximum number of datapath ports.

Pravin Shelar pshelar at nicira.com
Tue Feb 7 04:06:23 UTC 2012


On Mon, Feb 6, 2012 at 6:18 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Thu, Feb 2, 2012 at 6:58 AM, Pravin B Shelar <pshelar at nicira.com> wrote:
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 824791d..cc238c4 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -248,7 +248,7 @@ static int do_output(struct datapath *dp, struct sk_buff *skb, int out_port)
>>        if (unlikely(!skb))
>>                return -ENOMEM;
>>
>> -       vport = rcu_dereference(dp->ports[out_port]);
>> +       vport = ovs_vport(dp, out_port);
>
> ovs_vport() and rcu_dereference() aren't equivalent because the latter
> verifies that rcu_read_lock is held.
>
ok.

>> @@ -1391,6 +1404,11 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
>>        }
>>        ovs_dp_set_net(dp, hold_net(sock_net(skb->sk)));
>>
>> +       err = flex_array_init(&dp->ports, sizeof(struct vport *),
>> +                             DP_MAX_PORTS, GFP_KERNEL|__GFP_ZERO);
>
> Please put spaces around the pipe.
>
ok.

>> diff --git a/datapath/datapath.h b/datapath/datapath.h
>> index b012a76..05f6de8 100644
>> --- a/datapath/datapath.h
>> +++ b/datapath/datapath.h
>> @@ -34,7 +34,10 @@
>>  #include "vlan.h"
>>  #include "vport.h"
>>
>> -#define DP_MAX_PORTS 1024
>> +/* Flex array limit. */
>> +#define DP_MAX_PORTS (FLEX_ARRAY_ELEMENTS_PER_PART(sizeof(struct vport *)) *  \
>> +                     FLEX_ARRAY_NR_BASE_PTRS)
>
> In most situations the number of ports is actually small enough that
> it's possible to fit them all in the single page version of flex
> array.  It would be nice if we could take advantage of that, although
> that depends on what the other changes look like since we'll need to
> be able to pivot from the single page to multiple pages safely if the
> need arises.
>
Are you saying we shld do expansion like flow-table for vport table ?

>> +static inline struct vport *ovs_vport(const struct datapath *dp, int id)
>> +{
>> +       return flex_array_get_ptr(&dp->ports, id);
>> +}
>> +
>> +static inline struct vport *ovs_vport_check(const struct datapath *dp, int id)
>> +{
>> +       WARN_ON(!rcu_read_lock_held() && !rtnl_is_locked());
>> +       return ovs_vport(dp, id);
>> +}
>> +
>> +static inline struct vport *ovs_vport_protected(const struct datapath *dp, int id)
>> +{
>> +       ASSERT_RTNL();
>> +       return ovs_vport(dp, id);
>> +}
>
> These function names aren't as descriptive as the ones they replace -
> it would be good to include rcu or rtnl in the name to indicate what
> condition is verified.

ok.



More information about the dev mailing list