netdev - Re: [net-next] openvswitch: Simplify do_execute

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAOrHB_C3C_pcPpS=TSvKD0pF8eKRZMVgCK6eNKfbJnT_-xs1xw@mail.gmail.com>
Date:   Fri, 27 Jan 2017 12:42:27 -0800
From:   Pravin Shelar <pshelar@....org>
To:     Andy Zhou <azhou@....org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [net-next] openvswitch: Simplify do_execute_actions().

On Wed, Jan 25, 2017 at 9:24 PM, Andy Zhou <azhou@....org> wrote:
> do_execute_actions() implements a worthwhile optimization: in case
> an output action is the last action in an action list, skb_clone()
> can be avoided by outputing the current skb. However, the
> implementation is more complicated than necessary.  This patch
> simplify this logic.
>
> Signed-off-by: Andy Zhou <azhou@....org>
> ---
>  net/openvswitch/actions.c | 40 +++++++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 514f7bc..3866608 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -830,6 +830,9 @@ static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
>  {
>         struct vport *vport = ovs_vport_rcu(dp, out_port);
>
> +       if (unlikely(!skb))
> +               return;
> +
Patch looks good to me. But I wanted to know if you considered moving
this check to do_execute_actions() in case skb-clone is done? This way
we can avoid this unlikely check from likely case :)


>         if (likely(vport)) {
>                 u16 mru = OVS_CB(skb)->mru;
>                 u32 cutlen = OVS_CB(skb)->cutlen;
> @@ -1141,12 +1144,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>                               struct sw_flow_key *key,
>                               const struct nlattr *attr, int len)
>  {
> -       /* Every output action needs a separate clone of 'skb', but the common
> -        * case is just a single output action, so that doing a clone and
> -        * then freeing the original skbuff is wasteful.  So the following code
> -        * is slightly obscure just to avoid that.
> -        */
> -       int prev_port = -1;
>         const struct nlattr *a;
>         int rem;
>
> @@ -1154,20 +1151,25 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>              a = nla_next(a, &rem)) {
>                 int err = 0;
>
> -               if (unlikely(prev_port != -1)) {
> -                       struct sk_buff *out_skb = skb_clone(skb, GFP_ATOMIC);
> +               switch (nla_type(a)) {
> +               case OVS_ACTION_ATTR_OUTPUT: {
> +                       int port = nla_get_u32(a);
>
> -                       if (out_skb)
> -                               do_output(dp, out_skb, prev_port, key);
> +                       /* Every output action needs a separate clone
> +                        * of 'skb', In case the output action is the
> +                        * last action, cloning can be avoided.
> +                        */
> +                       if (nla_is_last(a, rem)) {
> +                               do_output(dp, skb, port, key);
> +                               /* 'skb' has been used for output.
> +                                */
> +                               return 0;
> +                       }
>
> +                       do_output(dp, skb_clone(skb, GFP_ATOMIC), port, key);
>                         OVS_CB(skb)->cutlen = 0;
> -                       prev_port = -1;
> -               }
> -
> -               switch (nla_type(a)) {
> -               case OVS_ACTION_ATTR_OUTPUT:
> -                       prev_port = nla_get_u32(a);
>                         break;
> +               }
>
>                 case OVS_ACTION_ATTR_TRUNC: {
>                         struct ovs_action_trunc *trunc = nla_data(a);