Thanks. Pushed with changes suggested.
On Tue, Jul 22, 2014 at 3:00 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Mon, Jul 21, 2014 at 1:39 PM, Andy Zhou <az...@nicira.com> wrote:
>> o get_dp_rcu() requires its caller to hold rcu lock.
>> o get_dp() requires its caller to hold either ovs lock or
>> rcu lock.
>
> Can you add why we are doing this change to the commit msg.
>
>> Signed-off-by: Andy Zhou <az...@nicira.com>
>>
>> ---
>> v1->v2: Reduce APIs from 4 to 2.
>> ---
>> datapath/datapath.c | 27 +++++++++++++++++++--------
>> 1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index 065356f..ba65947 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;
> There is no need to define dp here, just return NULL.
>
>> +}
>> +
>> +/* The caller must hold either ovs_mutex or rcu_read_lock to keep the
>> + * returned dp pointer valid. */
>> +static inline struct datapath *get_dp(struct net *net, int dp_ifindex)
>> +{
>> + struct datapath *dp;
>> +
>> + WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_ovsl_is_held());
>> + 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;
>> @@ -1252,7 +1263,7 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb,
>> struct netlink_callback *cb)
>> struct datapath *dp;
>>
>> 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);
>> if (!dp) {
>> rcu_read_unlock();
>> return -ENODEV;
>> @@ -2019,7 +2030,7 @@ static int ovs_vport_cmd_dump(struct sk_buff *skb,
>> struct netlink_callback *cb)
>> int i, j = 0;
>>
>> 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);
>> if (!dp) {
>> rcu_read_unlock();
>> return -ENODEV;
>
> otherwise looks good.
>
> Acked-by: Pravin B Shelar <pshe...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
