On Mon, Feb 6, 2012 at 6:18 PM, Jesse Gross <[email protected]> wrote:
> On Thu, Feb 2, 2012 at 6:58 AM, Pravin B Shelar <[email protected]> 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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
