[ovs-dev,v2,2/2] ofproto-dpif-xlate: optimize 100% sampling

For 100% sampling, no need to use sample action. Just use
userspace action for optimizing.

Signed-off-by: Benli Ye <daniely@vmware.com>
---
 ofproto/ofproto-dpif-xlate.c | 26 +++++++++++++++++---------
 tests/ofproto-dpif.at        | 10 +++++-----
 2 files changed, 22 insertions(+), 14 deletions(-)

> On Dec 22, 2016, at 9:58 PM, Benli Ye <daniely@vmware.com> wrote:
> 
> For 100% sampling, no need to use sample action. Just use
> userspace action for optimizing.
> 
> Signed-off-by: Benli Ye <daniely@vmware.com>
> ---
> ofproto/ofproto-dpif-xlate.c | 26 +++++++++++++++++---------
> tests/ofproto-dpif.at        | 10 +++++-----
> 2 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 3eb94ac..a133e16 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2506,18 +2506,24 @@ compose_sample_action(struct xlate_ctx *ctx,
>                       const odp_port_t tunnel_out_port,
>                       bool include_actions)
> {
> +    bool isSample = false;

OVS CodingStyle recommends underscores to separate words in identifier names:

"Naming
------

- Use names that explain the purpose of a function or object.

- Use underscores to separate words in an identifier: ``multi_word_name``."


> +    size_t sample_offset, actions_offset;
> +
>     if (probability == 0) {
>         /* No need to generate sampling or the inner action. */
>         return 0;
>     }
> 
> -    size_t sample_offset = nl_msg_start_nested(ctx->odp_actions,
> -                                               OVS_ACTION_ATTR_SAMPLE);
> -
> -    nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, probability);
> -
> -    size_t actions_offset = nl_msg_start_nested(ctx->odp_actions,
> -                                                OVS_SAMPLE_ATTR_ACTIONS);
> +    /* No need to generate sample action for 100% sampling rate. */
> +    if (probability < UINT32_MAX) {


Maybe like this:

  bool using_sample = probability < UINT32_MAX;

  if (using_sample) {

> +       sample_offset = nl_msg_start_nested(ctx->odp_actions,
> +                                           OVS_ACTION_ATTR_SAMPLE);
> +       nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
> +                      probability);
> +       actions_offset = nl_msg_start_nested(ctx->odp_actions,
> +                                            OVS_SAMPLE_ATTR_ACTIONS);
> +       isSample = true;
> +    }
> 
>     odp_port_t odp_port = ofp_port_to_odp_port(
>         ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
> @@ -2528,8 +2534,10 @@ compose_sample_action(struct xlate_ctx *ctx,
>                                                  include_actions,
>                                                  ctx->odp_actions);
> 
> -    nl_msg_end_nested(ctx->odp_actions, actions_offset);
> -    nl_msg_end_nested(ctx->odp_actions, sample_offset);
> +    if (isSample) {
> +       nl_msg_end_nested(ctx->odp_actions, actions_offset);
> +       nl_msg_end_nested(ctx->odp_actions, sample_offset);
> +    }
> 
>     return cookie_offset;
> }
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 5f594be..e35a806 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6245,7 +6245,7 @@ for i in `seq 1 3`; do
> done
> AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
> flow-dump from non-dpdk interfaces:
> -packets:2, bytes:68, used:0.001s, actions:sample(sample=100.0%,actions(userspace(pid=0,ipfix(output_port=4294967295))))
> +packets:2, bytes:68, used:0.001s, actions:userspace(pid=0,ipfix(output_port=4294967295))
> ])
> 
> dnl Remove the IPFIX configuration.
> @@ -6337,7 +6337,7 @@ for i in `seq 1 3`; do
> done
> AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
> flow-dump from non-dpdk interfaces:
> -packets:2, bytes:68, used:0.001s, actions:sample(sample=100.0%,actions(userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)))),2
> +packets:2, bytes:68, used:0.001s, actions:userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)),2
> ])
> 
> dnl Remove the flow which contains sample action.
> @@ -6385,7 +6385,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
> 
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
> AT_CHECK([tail -1 stdout], [0], [dnl
> -Datapath actions: sample(sample=100.0%,actions(userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)))),set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,tos=0x1,ttl=64,flags(df|key))),1
> +Datapath actions: userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)),set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,tos=0x1,ttl=64,flags(df|key))),1
> ])
> 
> dnl Remove the flow which contains sample action.
> @@ -6403,7 +6403,7 @@ AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(3),eth(src=50:54:00:00:00:
> dnl Make sure flow sample action in datapath is behind set tunnel
> dnl action at egress point of tunnel port.
> AT_CHECK([tail -1 stdout], [0], [dnl
> -Datapath actions: set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,tos=0x1,ttl=64,flags(df|key))),sample(sample=100.0%,actions(userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=1),tunnel_out_port=1))),1
> +Datapath actions: set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,tos=0x1,ttl=64,flags(df|key))),userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=1),tunnel_out_port=1),1
> ])
> 
> dnl Remove the flow which contains sample action.
> @@ -6421,7 +6421,7 @@ AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(3),eth(src=50:54:00:00:00:
> dnl Make sure flow sample action in datapath is behind set tunnel
> dnl action at egress point of tunnel port.
> AT_CHECK([tail -1 stdout], [0], [dnl
> -Datapath actions: set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,tos=0x1,ttl=64,flags(df|key))),sample(sample=100.0%,actions(userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=1),tunnel_out_port=1))),1,set(tunnel(tun_id=0x6,src=2.2.2.3,dst=1.1.1.2,tos=0x1,ttl=64,tp_dst=7471,flags(df|key))),sample(sample=100.0%,actions(userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=7471),tunnel_out_port=7471))),7471
> +Datapath actions: set(tunnel(tun_id=0x5,src=2.2.2.2,dst=1.1.1.1,tos=0x1,ttl=64,flags(df|key))),userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=1),tunnel_out_port=1),1,set(tunnel(tun_id=0x6,src=2.2.2.3,dst=1.1.1.2,tos=0x1,ttl=64,tp_dst=7471,flags(df|key))),userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=7471),tunnel_out_port=7471),7471
> ])
> 
> dnl Remove the flow which contains sample action.
> -- 
> 1.9.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Tue, Jan 03, 2017 at 05:27:50PM -0800, Jarno Rajahalme wrote:
> 
> > On Dec 22, 2016, at 9:58 PM, Benli Ye <daniely@vmware.com> wrote:
> > 
> > For 100% sampling, no need to use sample action. Just use
> > userspace action for optimizing.
> > 
> > Signed-off-by: Benli Ye <daniely@vmware.com>
> > ---
> > ofproto/ofproto-dpif-xlate.c | 26 +++++++++++++++++---------
> > tests/ofproto-dpif.at        | 10 +++++-----
> > 2 files changed, 22 insertions(+), 14 deletions(-)
> > 
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 3eb94ac..a133e16 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -2506,18 +2506,24 @@ compose_sample_action(struct xlate_ctx *ctx,
> >                       const odp_port_t tunnel_out_port,
> >                       bool include_actions)
> > {
> > +    bool isSample = false;
> 
> OVS CodingStyle recommends underscores to separate words in identifier names:
> 
> "Naming
> ------
> 
> - Use names that explain the purpose of a function or object.
> 
> - Use underscores to separate words in an identifier: ``multi_word_name``."
> 
> 
> > +    size_t sample_offset, actions_offset;
> > +
> >     if (probability == 0) {
> >         /* No need to generate sampling or the inner action. */
> >         return 0;
> >     }
> > 
> > -    size_t sample_offset = nl_msg_start_nested(ctx->odp_actions,
> > -                                               OVS_ACTION_ATTR_SAMPLE);
> > -
> > -    nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, probability);
> > -
> > -    size_t actions_offset = nl_msg_start_nested(ctx->odp_actions,
> > -                                                OVS_SAMPLE_ATTR_ACTIONS);
> > +    /* No need to generate sample action for 100% sampling rate. */
> > +    if (probability < UINT32_MAX) {
> 
> 
> Maybe like this:
> 
>   bool using_sample = probability < UINT32_MAX;
> 
>   if (using_sample) {

I applied this a few weeks back.  My change to it was remarkably similar
to your suggestion.