Re: [ovs-dev] [action upcall meters 5/5] ofproto: Meter slowpath action when action upcall meters are configured

[Andy Zhou]

On Thu, Apr 27, 2017 at 3:49 PM, Jarno Rajahalme  wrote:
> See comments below.
>
>   Jarno
>
>> On Apr 14, 2017, at 12:46 PM, Andy Zhou  wrote:
>>
>> If a slow path action is a controller action, meter it when the
>> controller meter is configured.  For other kinds of slow path actions,
>> meter it when the slowpath meter is configured.
>>
>> Note, this patch only considers the meters configuration of the
>> packet's input bridge, which may not be the same bridge that the
>> action is generated.
>>
>> Signed-off-by: Andy Zhou 
>> ---
>> ofproto/ofproto-dpif-upcall.c | 34 +++---
>> ofproto/ofproto-dpif-xlate.c  | 12 ++--
>> tests/ofproto-dpif.at | 31 +++
>> 3 files changed, 68 insertions(+), 9 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 3b28f9a22939..37f345b235b1 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -1018,7 +1018,8 @@ classify_upcall(enum dpif_upcall_type type, const 
>> struct nlattr *userdata)
>> static void
>> compose_slow_path(struct udpif *udpif, struct xlate_out *xout,
>>   const struct flow *flow, odp_port_t odp_in_port,
>> -  struct ofpbuf *buf)
>> +  struct ofpbuf *buf, uint32_t slowpath_meter_id,
>> +  uint32_t controller_meter_id)
>> {
>> union user_action_cookie cookie;
>> odp_port_t port;
>> @@ -1032,8 +1033,28 @@ compose_slow_path(struct udpif *udpif, struct 
>> xlate_out *xout,
>> ? ODPP_NONE
>> : odp_in_port;
>> pid = dpif_port_get_pid(udpif->dpif, port, flow_hash_5tuple(flow, 0));
>> +
>> +size_t offset;
>> +size_t ac_offset;
>> +uint32_t meter_id = xout->slow & SLOW_CONTROLLER ? controller_meter_id
>> + : slowpath_meter_id;
>> +
>> +if (meter_id != UINT32_MAX) {
>> +/* If slowpath meter is configured, generate clone(meter, userspace)
>> + * action.   */
>> +offset = nl_msg_start_nested(buf, OVS_ACTION_ATTR_SAMPLE);
>> +nl_msg_put_u32(buf, OVS_SAMPLE_ATTR_PROBABILITY, UINT32_MAX);
>> +ac_offset = nl_msg_start_nested(buf, OVS_SAMPLE_ATTR_ACTIONS);
>> +nl_msg_put_u32(buf, OVS_ACTION_ATTR_METER, meter_id);
>> +}
>> +
>> odp_put_userspace_action(pid, , sizeof cookie.slow_path,
>>  ODPP_NONE, false, buf);
>> +
>> +if (meter_id != UINT32_MAX) {
>> +nl_msg_end_nested(buf, ac_offset);
>> +nl_msg_end_nested(buf, offset);
>> +}
>> }
>>
>> /* If there is no error, the upcall must be destroyed with upcall_uninit()
>> @@ -1136,10 +1157,12 @@ upcall_xlate(struct udpif *udpif, struct upcall 
>> *upcall,
>> ofpbuf_use_const(>put_actions,
>>  odp_actions->data, odp_actions->size);
>> } else {
>> +uint32_t smid= upcall->ofproto->up.slowpath_meter_id;
>
> white space error.
Thanks, will fix.
>
>> +uint32_t cmid = upcall->ofproto->up.controller_meter_id;
>> /* upcall->put_actions already initialized by upcall_receive(). */
>> compose_slow_path(udpif, >xout, upcall->flow,
>>   upcall->flow->in_port.odp_port,
>> -  >put_actions);
>> +  >put_actions, smid, cmid);
>> }
>>
>> /* This function is also called for slow-pathed flows.  As we are only
>> @@ -1956,9 +1979,14 @@ revalidate_ukey__(struct udpif *udpif, const struct 
>> udpif_key *ukey,
>> }
>>
>> if (xoutp->slow) {
>> +struct ofproto_dpif *ofproto;
>> +ofproto = xlate_lookup_ofproto(udpif->backer, , NULL);
>> +uint32_t smid= ofproto->up.slowpath_meter_id;
>> +uint32_t cmid= ofproto->up.controller_meter_id;
>> +
>> ofpbuf_clear(odp_actions);
>> compose_slow_path(udpif, xoutp, , ctx.flow.in_port.odp_port,
>> -  odp_actions);
>> +  odp_actions, smid, cmid);
>> }
>>
>> if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, _mask, )
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 52e0d3f1b0bb..416012ab6930 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -2849,8 +2849,13 @@ compose_sample_action(struct xlate_ctx *ctx,
>> return 0;
>> }
>>
>> +/* If the slow path meter is configured by the controller,
>> + * Insert a meter action before the user space action.   */
>> +struct ofproto *ofproto = >xin->ofproto->up;
>> +uint32_t meter_id = ofproto->slowpath_meter_id;
>> +
>> /* No need to generate sample action for 100% sampling rate. */
>> -bool is_sample = probability < UINT32_MAX;
>> +bool is_sample = probability < UINT32_MAX || meter_id != UINT32_MAX;
>
> This seems to fix the problem in the previous patch.

Re: [ovs-dev] [action upcall meters 5/5] ofproto: Meter slowpath action when action upcall meters are configured

[Jarno Rajahalme]

See comments below.

  Jarno

> On Apr 14, 2017, at 12:46 PM, Andy Zhou  wrote:
> 
> If a slow path action is a controller action, meter it when the
> controller meter is configured.  For other kinds of slow path actions,
> meter it when the slowpath meter is configured.
> 
> Note, this patch only considers the meters configuration of the
> packet's input bridge, which may not be the same bridge that the
> action is generated.
> 
> Signed-off-by: Andy Zhou 
> ---
> ofproto/ofproto-dpif-upcall.c | 34 +++---
> ofproto/ofproto-dpif-xlate.c  | 12 ++--
> tests/ofproto-dpif.at | 31 +++
> 3 files changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 3b28f9a22939..37f345b235b1 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1018,7 +1018,8 @@ classify_upcall(enum dpif_upcall_type type, const 
> struct nlattr *userdata)
> static void
> compose_slow_path(struct udpif *udpif, struct xlate_out *xout,
>   const struct flow *flow, odp_port_t odp_in_port,
> -  struct ofpbuf *buf)
> +  struct ofpbuf *buf, uint32_t slowpath_meter_id,
> +  uint32_t controller_meter_id)
> {
> union user_action_cookie cookie;
> odp_port_t port;
> @@ -1032,8 +1033,28 @@ compose_slow_path(struct udpif *udpif, struct 
> xlate_out *xout,
> ? ODPP_NONE
> : odp_in_port;
> pid = dpif_port_get_pid(udpif->dpif, port, flow_hash_5tuple(flow, 0));
> +
> +size_t offset;
> +size_t ac_offset;
> +uint32_t meter_id = xout->slow & SLOW_CONTROLLER ? controller_meter_id
> + : slowpath_meter_id;
> +
> +if (meter_id != UINT32_MAX) {
> +/* If slowpath meter is configured, generate clone(meter, userspace)
> + * action.   */
> +offset = nl_msg_start_nested(buf, OVS_ACTION_ATTR_SAMPLE);
> +nl_msg_put_u32(buf, OVS_SAMPLE_ATTR_PROBABILITY, UINT32_MAX);
> +ac_offset = nl_msg_start_nested(buf, OVS_SAMPLE_ATTR_ACTIONS);
> +nl_msg_put_u32(buf, OVS_ACTION_ATTR_METER, meter_id);
> +}
> +
> odp_put_userspace_action(pid, , sizeof cookie.slow_path,
>  ODPP_NONE, false, buf);
> +
> +if (meter_id != UINT32_MAX) {
> +nl_msg_end_nested(buf, ac_offset);
> +nl_msg_end_nested(buf, offset);
> +}
> }
> 
> /* If there is no error, the upcall must be destroyed with upcall_uninit()
> @@ -1136,10 +1157,12 @@ upcall_xlate(struct udpif *udpif, struct upcall 
> *upcall,
> ofpbuf_use_const(>put_actions,
>  odp_actions->data, odp_actions->size);
> } else {
> +uint32_t smid= upcall->ofproto->up.slowpath_meter_id;

white space error.

> +uint32_t cmid = upcall->ofproto->up.controller_meter_id;
> /* upcall->put_actions already initialized by upcall_receive(). */
> compose_slow_path(udpif, >xout, upcall->flow,
>   upcall->flow->in_port.odp_port,
> -  >put_actions);
> +  >put_actions, smid, cmid);
> }
> 
> /* This function is also called for slow-pathed flows.  As we are only
> @@ -1956,9 +1979,14 @@ revalidate_ukey__(struct udpif *udpif, const struct 
> udpif_key *ukey,
> }
> 
> if (xoutp->slow) {
> +struct ofproto_dpif *ofproto;
> +ofproto = xlate_lookup_ofproto(udpif->backer, , NULL);
> +uint32_t smid= ofproto->up.slowpath_meter_id;
> +uint32_t cmid= ofproto->up.controller_meter_id;
> +
> ofpbuf_clear(odp_actions);
> compose_slow_path(udpif, xoutp, , ctx.flow.in_port.odp_port,
> -  odp_actions);
> +  odp_actions, smid, cmid);
> }
> 
> if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, _mask, )
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 52e0d3f1b0bb..416012ab6930 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2849,8 +2849,13 @@ compose_sample_action(struct xlate_ctx *ctx,
> return 0;
> }
> 
> +/* If the slow path meter is configured by the controller,
> + * Insert a meter action before the user space action.   */
> +struct ofproto *ofproto = >xin->ofproto->up;
> +uint32_t meter_id = ofproto->slowpath_meter_id;
> +
> /* No need to generate sample action for 100% sampling rate. */
> -bool is_sample = probability < UINT32_MAX;
> +bool is_sample = probability < UINT32_MAX || meter_id != UINT32_MAX;

This seems to fix the problem in the previous patch.

> size_t sample_offset, actions_offset;
> if (is_sample) {
> sample_offset = nl_msg_start_nested(ctx->odp_actions,
> @@ -2861,11 +2866,6 @@ compose_sample_action(struct xlate_ctx *ctx,
> 

[ovs-dev] [action upcall meters 5/5] ofproto: Meter slowpath action when action upcall meters are configured

[Andy Zhou]

If a slow path action is a controller action, meter it when the
controller meter is configured.  For other kinds of slow path actions,
meter it when the slowpath meter is configured.

Note, this patch only considers the meters configuration of the
packet's input bridge, which may not be the same bridge that the
action is generated.

Signed-off-by: Andy Zhou 
---
 ofproto/ofproto-dpif-upcall.c | 34 +++---
 ofproto/ofproto-dpif-xlate.c  | 12 ++--
 tests/ofproto-dpif.at | 31 +++
 3 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 3b28f9a22939..37f345b235b1 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1018,7 +1018,8 @@ classify_upcall(enum dpif_upcall_type type, const struct 
nlattr *userdata)
 static void
 compose_slow_path(struct udpif *udpif, struct xlate_out *xout,
   const struct flow *flow, odp_port_t odp_in_port,
-  struct ofpbuf *buf)
+  struct ofpbuf *buf, uint32_t slowpath_meter_id,
+  uint32_t controller_meter_id)
 {
 union user_action_cookie cookie;
 odp_port_t port;
@@ -1032,8 +1033,28 @@ compose_slow_path(struct udpif *udpif, struct xlate_out 
*xout,
 ? ODPP_NONE
 : odp_in_port;
 pid = dpif_port_get_pid(udpif->dpif, port, flow_hash_5tuple(flow, 0));
+
+size_t offset;
+size_t ac_offset;
+uint32_t meter_id = xout->slow & SLOW_CONTROLLER ? controller_meter_id
+ : slowpath_meter_id;
+
+if (meter_id != UINT32_MAX) {
+/* If slowpath meter is configured, generate clone(meter, userspace)
+ * action.   */
+offset = nl_msg_start_nested(buf, OVS_ACTION_ATTR_SAMPLE);
+nl_msg_put_u32(buf, OVS_SAMPLE_ATTR_PROBABILITY, UINT32_MAX);
+ac_offset = nl_msg_start_nested(buf, OVS_SAMPLE_ATTR_ACTIONS);
+nl_msg_put_u32(buf, OVS_ACTION_ATTR_METER, meter_id);
+}
+
 odp_put_userspace_action(pid, , sizeof cookie.slow_path,
  ODPP_NONE, false, buf);
+
+if (meter_id != UINT32_MAX) {
+nl_msg_end_nested(buf, ac_offset);
+nl_msg_end_nested(buf, offset);
+}
 }
 
 /* If there is no error, the upcall must be destroyed with upcall_uninit()
@@ -1136,10 +1157,12 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
 ofpbuf_use_const(>put_actions,
  odp_actions->data, odp_actions->size);
 } else {
+uint32_t smid= upcall->ofproto->up.slowpath_meter_id;
+uint32_t cmid = upcall->ofproto->up.controller_meter_id;
 /* upcall->put_actions already initialized by upcall_receive(). */
 compose_slow_path(udpif, >xout, upcall->flow,
   upcall->flow->in_port.odp_port,
-  >put_actions);
+  >put_actions, smid, cmid);
 }
 
 /* This function is also called for slow-pathed flows.  As we are only
@@ -1956,9 +1979,14 @@ revalidate_ukey__(struct udpif *udpif, const struct 
udpif_key *ukey,
 }
 
 if (xoutp->slow) {
+struct ofproto_dpif *ofproto;
+ofproto = xlate_lookup_ofproto(udpif->backer, , NULL);
+uint32_t smid= ofproto->up.slowpath_meter_id;
+uint32_t cmid= ofproto->up.controller_meter_id;
+
 ofpbuf_clear(odp_actions);
 compose_slow_path(udpif, xoutp, , ctx.flow.in_port.odp_port,
-  odp_actions);
+  odp_actions, smid, cmid);
 }
 
 if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, _mask, )
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 52e0d3f1b0bb..416012ab6930 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2849,8 +2849,13 @@ compose_sample_action(struct xlate_ctx *ctx,
 return 0;
 }
 
+/* If the slow path meter is configured by the controller,
+ * Insert a meter action before the user space action.   */
+struct ofproto *ofproto = >xin->ofproto->up;
+uint32_t meter_id = ofproto->slowpath_meter_id;
+
 /* No need to generate sample action for 100% sampling rate. */
-bool is_sample = probability < UINT32_MAX;
+bool is_sample = probability < UINT32_MAX || meter_id != UINT32_MAX;
 size_t sample_offset, actions_offset;
 if (is_sample) {
 sample_offset = nl_msg_start_nested(ctx->odp_actions,
@@ -2861,11 +2866,6 @@ compose_sample_action(struct xlate_ctx *ctx,
  OVS_SAMPLE_ATTR_ACTIONS);
 }
 
-/* If the slow path meter is configured by the controller,
- * Insert a meter action before the user space action.   */
-struct ofproto *ofproto = >xin->ofproto->up;
-uint32_t meter_id = ofproto->slowpath_meter_id;
-
 if (meter_id != UINT32_MAX) {