Thanks for the review. Both points are valid.
I have just posted another version of the re-factoring that should address
the issues.
On Wed, Jul 16, 2014 at 9:30 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Tue, Jul 15, 2014 at 2:56 PM, Andy Zhou <az...@nicira.com> wrote:
>> The added get_dp_rcu() requires its caller to hold rcu lock.
>> get_dp() now requires caller to hold ovs lock.
>>
>> Sepearate APIs makes it clear on the intended use and the requirements
>> of its caller.
>>
>> Signed-off-by: Andy Zhou <az...@nicira.com>
>> ---
>> datapath/datapath.c | 23 +++++++++++++++++------
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index 065356f..56f5b72 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -140,19 +140,30 @@ static int queue_gso_packets(struct datapath *dp,
>> struct sk_buff *,
>> static int queue_userspace_packet(struct datapath *dp, struct sk_buff *,
>> const struct dp_upcall_info *);
>>
>> -/* Must be called with rcu_read_lock or ovs_mutex. */
>> -static struct datapath *get_dp(struct net *net, int dp_ifindex)
>> +/* Must be called with rcu_read_lock. */
>> +static struct datapath *get_dp_rcu(struct net *net, int dp_ifindex)
>> {
>> + struct net_device *dev = dev_get_by_index_rcu(net, dp_ifindex);
>> struct datapath *dp = NULL;
>> - struct net_device *dev;
>>
>> - rcu_read_lock();
>> - dev = dev_get_by_index_rcu(net, dp_ifindex);
>> if (dev) {
>> struct vport *vport = ovs_internal_dev_get_vport(dev);
>> if (vport)
>> dp = vport->dp;
>> }
>> +
>> + return dp;
>> +}
>> +
>> +/* The caller must hold ovs_mutex to keep the returned dp pointer valid. */
>> +static struct datapath *get_dp(struct net *net, int dp_ifindex)
>> +{
>> + struct datapath *dp = NULL;
>> +
> This assignment is not required.
>
>> + ASSERT_OVSL();
>> +
> There are call sites for get_dp() holding only rcu-read-lock where
> this assert fails.
>
>
>> + rcu_read_lock();
>> + dp = get_dp_rcu(net, dp_ifindex);
>> rcu_read_unlock();
>>
>> return dp;
>> @@ -595,7 +606,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb,
>> struct genl_info *info)
>> packet->mark = flow->key.phy.skb_mark;
>>
>> rcu_read_lock();
>> - dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> + dp = get_dp_rcu(sock_net(skb->sk), ovs_header->dp_ifindex);
>> err = -ENODEV;
>> if (!dp)
>> goto err_unlock;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
