O.K. I have send out a V2 that reduced the number of APIs from 4 to 2.
get_dp() as before, and get_dp_rcu(). V2 is a simpler change.
On Mon, Jul 21, 2014 at 11:04 AM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Wed, Jul 16, 2014 at 11:42 PM, Andy Zhou <az...@nicira.com> wrote:
>> Remove get_dp() API, it becomes get_dp__() which is for internal
>> use only. Added the following APIs to make it clear of the locking
>> requirement.
>>
>> o get_dp_rcu() requires its caller to hold rcu lock.
>> o get_dp_ovsl() requires its caller to hold ovs lock.
>> o get_dp_ovsl_rcu() requires its caller to hold either ovs lock or
>> rcu lock.
>>
>
> I think this is getting bit complicate. I am not sure if it is worth
> adding this complexity at this point.
>
>
>> Signed-off-by: Andy Zhou <az...@nicira.com>
>> ---
>> datapath/datapath.c | 53
>> ++++++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 38 insertions(+), 15 deletions(-)
>>
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index 065356f..f018bd4 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -140,24 +140,47 @@ 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;
>> +}
>> +
>> +static struct datapath *get_dp__(struct net *net, int dp_ifindex)
>> +{
>> + struct datapath *dp;
>> +
>> + rcu_read_lock();
>> + dp = get_dp_rcu(net, dp_ifindex);
>> rcu_read_unlock();
>>
>> return dp;
>> }
>>
>> +/* The caller must hold either ovs_mutex or rcu_read_lock to keep the
>> + * returned dp pointer valid. */
>> +static inline struct datapath *get_dp_ovsl_rcu(struct net *net,
>> + int dp_ifindex)
>> +{
>> + WARN_ON_ONCE(!rcu_read_lock_held() && !lockdep_ovsl_is_held());
>> + return get_dp__(net, dp_ifindex);
>> +}
>> +
>> +static struct datapath *get_dp_ovsl(struct net *net, int dp_ifindex)
>> +{
>> + ASSERT_OVSL();
>> + return get_dp__(net, dp_ifindex);
>> +}
>> +
>> /* Must be called with rcu_read_lock or ovs_mutex. */
>> const char *ovs_dp_name(const struct datapath *dp)
>> {
>> @@ -595,7 +618,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;
>> @@ -926,7 +949,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct
>> genl_info *info)
>> }
>>
>> ovs_lock();
>> - dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> + dp = get_dp_ovsl(sock_net(skb->sk), ovs_header->dp_ifindex);
>> if (unlikely(!dp)) {
>> error = -ENODEV;
>> goto err_unlock_ovs;
>> @@ -1076,7 +1099,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb,
>> struct genl_info *info)
>> }
>>
>> ovs_lock();
>> - dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> + dp = get_dp_ovsl(sock_net(skb->sk), ovs_header->dp_ifindex);
>> if (unlikely(!dp)) {
>> error = -ENODEV;
>> goto err_unlock_ovs;
>> @@ -1154,7 +1177,7 @@ static int ovs_flow_cmd_get(struct sk_buff *skb,
>> struct genl_info *info)
>> return err;
>>
>> ovs_lock();
>> - dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> + dp = get_dp_ovsl(sock_net(skb->sk), ovs_header->dp_ifindex);
>> if (!dp) {
>> err = -ENODEV;
>> goto unlock;
>> @@ -1199,7 +1222,7 @@ static int ovs_flow_cmd_del(struct sk_buff *skb,
>> struct genl_info *info)
>> }
>>
>> ovs_lock();
>> - dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> + dp = get_dp_ovsl(sock_net(skb->sk), ovs_header->dp_ifindex);
>> if (unlikely(!dp)) {
>> err = -ENODEV;
>> goto unlock;
>> @@ -1252,7 +1275,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;
>> @@ -1392,7 +1415,7 @@ static struct datapath *lookup_datapath(struct net
>> *net,
>> struct datapath *dp;
>>
>> if (!a[OVS_DP_ATTR_NAME])
>> - dp = get_dp(net, ovs_header->dp_ifindex);
>> + dp = get_dp_ovsl_rcu(net, ovs_header->dp_ifindex);
>> else {
>> struct vport *vport;
>>
>> @@ -1801,7 +1824,7 @@ static struct vport *lookup_vport(struct net *net,
>> if (port_no >= DP_MAX_PORTS)
>> return ERR_PTR(-EFBIG);
>>
>> - dp = get_dp(net, ovs_header->dp_ifindex);
>> + dp = get_dp_ovsl_rcu(net, ovs_header->dp_ifindex);
>> if (!dp)
>> return ERR_PTR(-ENODEV);
>>
>> @@ -1838,7 +1861,7 @@ static int ovs_vport_cmd_new(struct sk_buff *skb,
>> struct genl_info *info)
>> return -ENOMEM;
>>
>> ovs_lock();
>> - dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> + dp = get_dp_ovsl(sock_net(skb->sk), ovs_header->dp_ifindex);
>> err = -ENODEV;
>> if (!dp)
>> goto exit_unlock_free;
>> @@ -2019,7 +2042,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;
>> --
>> 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
