[ovs-dev] [PATCH 1/4] datapath: Remove recirc stack depth limit check

Andy Zhou azhou at nicira.com
Mon Aug 18 20:32:18 UTC 2014


This is what I should do if I understand your intention correctly:

1. Change this patch to remove all loop detection mechanisms.
2. Submit a separate patch to the compact code that check for a lower
xmit_recursion limit in rpl_dev_queue_xmit().

If this is not what you had in mind, please let me know your preference.

Thanks,

Andy

On Sun, Aug 17, 2014 at 8:00 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Fri, Aug 15, 2014 at 3:12 AM, Andy Zhou <azhou at nicira.com> wrote:
>> Future patches will change the recirc action implementation to not
>> using recursion. The stack depth detection is no longer necessary.
>>
> Once there is no recursion in OVS, only recursion left is recursion
> due to networking stack. networking stack controls recursion by
> xmit_recursion. But xmit_recursion limit is too high, So we can have
> ovs specific xmit_recursion limit in compat xmit code.
>
>> Signed-off-by: Andy Zhou <azhou at nicira.com>
>> ---
>>  datapath/actions.c  | 34 ++++++++++------------------------
>>  datapath/datapath.c |  6 +++---
>>  datapath/datapath.h |  4 ++--
>>  datapath/vport.c    |  2 +-
>>  4 files changed, 16 insertions(+), 30 deletions(-)
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index b16e0b2..bccf397 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -816,7 +816,7 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>>                 flow_key_clone(skb, &recirc_key);
>>
>>         flow_key_set_recirc_id(skb, nla_get_u32(a));
>> -       ovs_dp_process_packet(skb, true);
>> +       ovs_dp_process_packet(skb);
>>         return 0;
>>  }
>>
>> @@ -904,18 +904,11 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>  }
>>
>>  /* We limit the number of times that we pass into execute_actions()
>> - * to avoid blowing out the stack in the event that we have a loop.
>> - *
>> - * Each loop adds some (estimated) cost to the kernel stack.
>> - * The loop terminates when the max cost is exceeded.
>> - * */
>> -#define RECIRC_STACK_COST 1
>> -#define DEFAULT_STACK_COST 4
>> -/* Allow up to 4 regular services, and up to 3 recirculations */
>> -#define MAX_STACK_COST (DEFAULT_STACK_COST * 4 + RECIRC_STACK_COST * 3)
>> + *  * to avoid blowing out the stack in the event that we have a loop. */
>> +#define MAX_LOOPS 4
>>
>>  struct loop_counter {
>> -       u8 stack_cost;          /* loop stack cost. */
>> +       u8 count;               /* Count. */
>>         bool looping;           /* Loop detected? */
>>  };
>>
>> @@ -924,24 +917,22 @@ static DEFINE_PER_CPU(struct loop_counter, loop_counters);
>>  static int loop_suppress(struct datapath *dp, struct sw_flow_actions *actions)
>>  {
>>         if (net_ratelimit())
>> -               pr_warn("%s: flow loop detected, dropping\n",
>> -                               ovs_dp_name(dp));
>> +               pr_warn("%s: flow looped %d times, dropping\n",
>> +                               ovs_dp_name(dp), MAX_LOOPS);
>>         actions->actions_len = 0;
>>         return -ELOOP;
>>  }
>>
>>  /* Execute a list of actions against 'skb'. */
>> -int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb, bool recirc)
>> +int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
>>  {
>>         struct sw_flow_actions *acts = rcu_dereference(OVS_CB(skb)->flow->sf_acts);
>> -       const u8 stack_cost = recirc ? RECIRC_STACK_COST : DEFAULT_STACK_COST;
>>         struct loop_counter *loop;
>>         int error;
>>
>>         /* Check whether we've looped too much. */
>>         loop = &__get_cpu_var(loop_counters);
>> -       loop->stack_cost += stack_cost;
>> -       if (unlikely(loop->stack_cost > MAX_STACK_COST))
>> +       if (unlikely(++loop->count > MAX_LOOPS))
>>                 loop->looping = true;
>>         if (unlikely(loop->looping)) {
>>                 error = loop_suppress(dp, acts);
>> @@ -951,14 +942,9 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb, bool recirc)
>>
>>         error = do_execute_actions(dp, skb, acts->actions, acts->actions_len);
>>
>> -       /* Check whether sub-actions looped too much. */
>> -       if (unlikely(loop->looping))
>> -               error = loop_suppress(dp, acts);
>> -
>>  out_loop:
>> -       /* Decrement loop stack cost. */
>> -       loop->stack_cost -= stack_cost;
>> -       if (!loop->stack_cost)
>> +       /* Decrement loop counter. */
>> +       if (!--loop->count)
>>                 loop->looping = false;
>>
>>         return error;
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index 6131b2a..e477c72 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -251,7 +251,7 @@ void ovs_dp_detach_port(struct vport *p)
>>  }
>>
>>  /* Must be called with rcu_read_lock. */
>> -void ovs_dp_process_packet(struct sk_buff *skb, bool recirc)
>> +void ovs_dp_process_packet(struct sk_buff *skb)
>>  {
>>         const struct vport *p = OVS_CB(skb)->input_vport;
>>         struct sw_flow_key *pkt_key = OVS_CB(skb)->pkt_key;
>> @@ -281,7 +281,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, bool recirc)
>>         OVS_CB(skb)->flow = flow;
>>
>>         ovs_flow_stats_update(OVS_CB(skb)->flow, pkt_key->tp.flags, skb);
>> -       ovs_execute_actions(dp, skb, recirc);
>> +       ovs_execute_actions(dp, skb);
>>         stats_counter = &stats->n_hit;
>>
>>  out:
>> @@ -566,7 +566,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>>         OVS_CB(packet)->input_vport = input_vport;
>>
>>         local_bh_disable();
>> -       err = ovs_execute_actions(dp, packet, false);
>> +       err = ovs_execute_actions(dp, packet);
>>         local_bh_enable();
>>         rcu_read_unlock();
>>
>> diff --git a/datapath/datapath.h b/datapath/datapath.h
>> index cd2acbc..4874ab5 100644
>> --- a/datapath/datapath.h
>> +++ b/datapath/datapath.h
>> @@ -188,7 +188,7 @@ extern struct notifier_block ovs_dp_device_notifier;
>>  extern struct genl_family dp_vport_genl_family;
>>  extern struct genl_multicast_group ovs_dp_vport_multicast_group;
>>
>> -void ovs_dp_process_packet(struct sk_buff *, bool recirc);
>> +void ovs_dp_process_packet(struct sk_buff *c);
>>  void ovs_dp_detach_port(struct vport *);
>>  int ovs_dp_upcall(struct datapath *, struct sk_buff *,
>>                   const struct dp_upcall_info *);
>> @@ -197,7 +197,7 @@ const char *ovs_dp_name(const struct datapath *dp);
>>  struct sk_buff *ovs_vport_cmd_build_info(struct vport *, u32 portid, u32 seq,
>>                                          u8 cmd);
>>
>> -int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb, bool recirc);
>> +int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb);
>>  void ovs_dp_notify_wq(struct work_struct *work);
>>
>>  #define OVS_NLERR(fmt, ...)                                    \
>> diff --git a/datapath/vport.c b/datapath/vport.c
>> index c44182c..4c68bae 100644
>> --- a/datapath/vport.c
>> +++ b/datapath/vport.c
>> @@ -495,7 +495,7 @@ void ovs_vport_receive(struct vport *vport, struct sk_buff *skb,
>>                 return;
>>         }
>>
>> -       ovs_dp_process_packet(skb, false);
>> +       ovs_dp_process_packet(skb);
>>  }
>>
>>  /**
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list