netdev - Re: [net-next sample action optimization v3 4/4] Openvswitch: Refactor sample and recirc actions implementation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 18 Mar 2017 12:22:39 -0700
From:   Pravin Shelar <pshelar@....org>
To:     Andy Zhou <azhou@....org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Joe Stringer <joe@....org>
Subject: Re: [net-next sample action optimization v3 4/4] Openvswitch:
 Refactor sample and recirc actions implementation

On Thu, Mar 16, 2017 at 3:48 PM, Andy Zhou <azhou@....org> wrote:
> Added clone_execute() that both the sample and the recirc
> action implementation can use.
>
> Signed-off-by: Andy Zhou <azhou@....org>
> ---
>  net/openvswitch/actions.c | 175 ++++++++++++++++++++++++----------------------
>  1 file changed, 92 insertions(+), 83 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 3529f7b..e38fa7f 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -44,10 +44,6 @@
>  #include "conntrack.h"
>  #include "vport.h"
>
> -static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> -                             struct sw_flow_key *key,
> -                             const struct nlattr *attr, int len);
> -
>  struct deferred_action {
>         struct sk_buff *skb;
>         const struct nlattr *actions;
> @@ -166,6 +162,12 @@ static bool is_flow_key_valid(const struct sw_flow_key *key)
>         return !(key->mac_proto & SW_FLOW_KEY_INVALID);
>  }
>
> +static int clone_execute(struct datapath *dp, struct sk_buff *skb,
> +                        struct sw_flow_key *key,
> +                        u32 recirc_id,
> +                        const struct nlattr *actions, int len,
> +                        bool last, bool clone_flow_key);
> +
With this function the diff stat looks much better.

...
...
> +/* Execute the actions on the clone of the packet. The effect of the
> + * execution does not affect the original 'skb' nor the original 'key'.
> + *
> + * The execution may be deferred in case the actions can not be executed
> + * immediately.
> + */
> +static int clone_execute(struct datapath *dp, struct sk_buff *skb,
> +                        struct sw_flow_key *key, u32 recirc_id,
> +                        const struct nlattr *actions, int len,
> +                        bool last, bool clone_flow_key)
> +{
> +       bool is_sample = actions;
Standard practice is use !! to convert pointer to boolean.
I think this function does not need to know about sample action. So we
can rename the boolean to have_actions or something similar.

> +       struct deferred_action *da;
> +       struct sw_flow_key *clone;
> +       int err = 0;
> +
> +       skb = last ? skb : skb_clone(skb, GFP_ATOMIC);
> +       if (!skb) {
> +               /* Out of memory, skip this action.
> +                */
> +               return 0;
> +       }
> +
> +       /* In case the sample actions won't change the 'key',
> +        * current key can be used directly to execute sample actions.
> +        * Otherwise, allocate a new key from the
> +        * next recursion level of 'flow_keys'. If
> +        * successful, execute the sample actions without
> +        * deferring.
> +        */
> +       if (is_sample && clone_flow_key)
> +               __this_cpu_inc(exec_actions_level);
> +
There is no need to increment actions level up here. it is only
required for do_execute_actions(). ovs_dp_process_packet() already
does it.


> +       clone = clone_flow_key ? clone_key(key) : key;
> +       if (clone) {
> +               if (is_sample) {
> +                       err = do_execute_actions(dp, skb, clone,
> +                                                actions, len);
> +               } else {
> +                       clone->recirc_id = recirc_id;
> +                       ovs_dp_process_packet(skb, clone);
> +               }
> +       }
wont this execute action twice, once here and again in deferred actions list?

> +
> +       if (is_sample && clone_flow_key)
> +               __this_cpu_dec(exec_actions_level);
> +
> +       /* Out of 'flow_keys' space. Defer them */
> +       da = add_deferred_actions(skb, key, actions, len);
> +       if (da) {
> +               if (!is_sample) {
> +                       key = &da->pkt_key;
> +                       key->recirc_id = recirc_id;
> +               }
> +       } else {
> +               /* Drop the SKB and log an error. */
> +               kfree_skb(skb);
> +
> +               if (net_ratelimit()) {
> +                       if (is_sample) {
> +                               pr_warn("%s: deferred action limit reached, drop sample action\n",
> +                                       ovs_dp_name(dp));
> +                       } else {
> +                               pr_warn("%s: deferred action limit reached, drop recirc action\n",
> +                                       ovs_dp_name(dp));
> +                       }
> +               }
> +       }
> +
> +       return err;
> +}
> +
>  static void process_deferred_actions(struct datapath *dp)
>  {
>         struct action_fifo *fifo = this_cpu_ptr(action_fifos);
> --
> 1.8.3.1
>