On 7/11/25 2:40 PM, Eelco Chaudron wrote:
> When a meter action is encountered and stored in the auxiliary
> structure, and subsequently, a non-meter action is processed
> within a nested list during callback execution, an infinite
> loop is triggered.
>
> This patch maintains the current behavior but stores all
> required meter actions in an ofpbuf for deferred execution.
>
> Fixes: 076caa2fb077 ("ofproto: Meter translation.")
> Signed-off-by: Eelco Chaudron <echau...@redhat.com>
> ---
> lib/dpif.c | 65 +++++++++++++++++--------------------------
> tests/ofproto-dpif.at | 41 +++++++++++++++++++++++++++
> 2 files changed, 66 insertions(+), 40 deletions(-)
Hi, Eelco. Thanks for the patch!
It appears to be solving the same problem as:
https://patchwork.ozlabs.org/project/openvswitch/patch/20250506022337.3242-1-danieldin...@gmail.com/
Please, at least, add a link to it as Reported-at. The solution seems
fairly different and you also have a proper test implemented, and there
were no updates for the previous patch in the past two months, so I won't
insist on getting the previous version, even though it was submitted first.
See some comments below.
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index a064f717f..0fd3d880c 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1163,7 +1163,7 @@ struct dpif_execute_helper_aux {
> struct dpif *dpif;
> const struct flow *flow;
> int error;
> - const struct nlattr *meter_action; /* Non-NULL, if have a meter action.
> */
> + struct ofpbuf meter_actions;
> };
>
> /* This is called for actions that need the context of the datapath to be
> @@ -1180,10 +1180,14 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
>
> switch ((enum ovs_action_attr)type) {
> case OVS_ACTION_ATTR_METER:
> - /* Maintain a pointer to the first meter action seen. */
> - if (!aux->meter_action) {
> - aux->meter_action = action;
> - }
> + /* XXX: This code collects meter actions since the last action
> + * execution via the datapath to be executed right before the
> + * current action that needs to be executed by the datapath.
> + * This is only an approximation, but better than nothing.
> + * Fundamentally, we should have a mechanism by which the
> + * datapath could return the result of the meter action so that
> + * we could execute them at the right order. */
> + ofpbuf_put(&aux->meter_actions, action, NLA_ALIGN(action->nla_len));
> break;
>
> case OVS_ACTION_ATTR_CT:
> @@ -1196,43 +1200,22 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
> case OVS_ACTION_ATTR_SAMPLE:
> case OVS_ACTION_ATTR_RECIRC: {
> struct dpif_execute execute;
> - struct ofpbuf execute_actions;
> - uint64_t stub[256 / 8];
> struct pkt_metadata *md = &packet->md;
>
> - if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_action) {
> - ofpbuf_use_stub(&execute_actions, stub, sizeof stub);
> -
> - if (aux->meter_action) {
> - const struct nlattr *a = aux->meter_action;
> -
> - /* XXX: This code collects meter actions since the last
> action
> - * execution via the datapath to be executed right before the
> - * current action that needs to be executed by the datapath.
> - * This is only an approximation, but better than nothing.
> - * Fundamentally, we should have a mechanism by which the
> - * datapath could return the result of the meter action so
> that
> - * we could execute them at the right order. */
> - do {
> - ofpbuf_put(&execute_actions, a, NLA_ALIGN(a->nla_len));
> - /* Find next meter action before 'action', if any. */
> - do {
> - a = nl_attr_next(a);
> - } while (a != action &&
> - nl_attr_type(a) != OVS_ACTION_ATTR_METER);
> - } while (a != action);
> - }
> + if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_actions.size) {
> +
The empty line here is not needed.
> + struct ofpbuf *execute_actions = &aux->meter_actions;
>
> /* The Linux kernel datapath throws away the tunnel information
> * that we supply as metadata. We have to use a "set" action to
> * supply it. */
> if (flow_tnl_dst_is_set(&md->tunnel)) {
> - odp_put_tunnel_action(&md->tunnel, &execute_actions, NULL);
> + odp_put_tunnel_action(&md->tunnel, execute_actions, NULL);
> }
> - ofpbuf_put(&execute_actions, action, NLA_ALIGN(action->nla_len));
> + ofpbuf_put(execute_actions, action, NLA_ALIGN(action->nla_len));
>
> - execute.actions = execute_actions.data;
> - execute.actions_len = execute_actions.size;
> + execute.actions = execute_actions->data;
> + execute.actions_len = execute_actions->size;
> } else {
> execute.actions = action;
> execute.actions_len = NLA_ALIGN(action->nla_len);
> @@ -1264,12 +1247,8 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
>
> dp_packet_delete(clone);
>
> - if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_action) {
> - ofpbuf_uninit(&execute_actions);
> -
> - /* Do not re-use the same meters for later output actions. */
> - aux->meter_action = NULL;
> - }
> + /* Do not re-use the same meters for later output actions. */
> + ofpbuf_reinit(&aux->meter_actions, 0);
The comment here is a bit misleading, because the ofpbuf was already reused
for other purposes and contains more than just meters.
I'd suggest to move this re-init inside the if condition that steals the
data, and actually use ofpbuf_steal_data() while pointing execute.actions
to this buffer to avoid any confusion.
> break;
> }
>
> @@ -1306,14 +1285,20 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
> static int
> dpif_execute_with_help(struct dpif *dpif, struct dpif_execute *execute)
> {
> - struct dpif_execute_helper_aux aux = {dpif, execute->flow, 0, NULL};
> + struct dpif_execute_helper_aux aux = {
> + .dpif = dpif,
> + .flow = execute->flow,
> + .error = 0
> + };
> struct dp_packet_batch pb;
>
> COVERAGE_INC(dpif_execute_with_help);
>
> + ofpbuf_init(&aux.meter_actions, 0);
> dp_packet_batch_init_packet(&pb, execute->packet);
> odp_execute_actions(&aux, &pb, false, execute->actions,
> execute->actions_len, dpif_execute_helper_cb);
> + ofpbuf_uninit(&aux.meter_actions);
> return aux.error;
> }
>
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 7930b3f29..702fb8d31 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6621,6 +6621,47 @@ This flow is handled by the userspace slow path
> because it:
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - meter with slow-path action])
> +
> +OVS_VSWITCHD_START([ dnl
> + add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \
> + add-port br0 p1 -- set Interface p1 type=dummy ofport_request=2])
add_of_ports --pcap br0 1 2
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-meter br0 \
> + 'meter=1 pktps burst stats bands=type=drop rate=100 burst_size=100'])
> +AT_CHECK([ovs-vsctl -- set Interface p0 options:tx_pcap=p0.pcap])
> +AT_CHECK([ovs-vsctl -- set Interface p1 options:tx_pcap=p1.pcap])
Not needed with the above add_of_ports.
> +
> +AT_CHECK([ovs-appctl vlog/set dpif:dbg])
> +
> +dnl Hex bytes for the following packet:
> +dnl 00:00:00:00:00:0c -> ff:ff:ff:ff:ff:ff
> +dnl ARP 42 Who has 192.168.0.1? Tell 192.168.0.100
> +eth="ffffffffffff0000000000ec0806"
> +arp="0001080006040001000c29abcdefc0a80064000000000000c0a80001"
> +packet="${eth}${arp}"
This should be done with 'ovs-ofctl compose-packet --bare'.
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 dnl
> +
> "table=1,eth_type=0x0806,actions=meter:1,clone(set_field:2->arp_op,output(port=p1,max_len=128)),output:p0"])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow15 packet-out br0 CONTROLLER \
> + "meter:1,resubmit(,1)" "$packet"])
> +
> +# Verify the orinigal packet is received on port 0.
> +OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep -E "$packet" | wc -l` -ge 1])
> +
> +dnl Turn the packet defenition into a reply and verify it's received on port
> 1.
> +packet=$(echo "$packet" | sed 's/\(.\{36\}\)01/\102/')
And another compose-packet here.
> +OVS_WAIT_UNTIL([test `ovs-pcap p1.pcap | grep -E "$packet" | wc -l` -ge 1])
It's a dummy datapath, we know the exact number of packets, also grep -c can
count,
and -E is probably not needed.
> +
> +dnl Make sure all meters, and outputs datapath actions get executed.
> +OVS_WAIT_UNTIL([grep -E "sub-execute meter\(0\),meter\(0\),. on packet arp"
> dnl
> + ovs-vswitchd.log])
> +OVS_WAIT_UNTIL([grep -E "sub-execute . on packet arp" ovs-vswitchd.log])
We know exact port numbers, so may avoid -E and escaping of the parenthesis.
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
No need for the extra line.
>
> dnl CHECK_CONTINUATION(TITLE, N_PORTS0, N_PORTS1, ACTIONS0, ACTIONS1,
> [EXTRA_SETUP])
> dnl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
