On 14 Jul 2025, at 18:42, Ilya Maximets wrote:
> 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.
Thank for the review Ilya. You are right, I should have added the reported-at.
Will do this in V2.
> See some comments below.
All seem valid, but one, as noted below, will resolve the rest in v2.
>>
>> 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.
ACK
>> + 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.
I tried your suggestion, and the code became harder to understand/read.
Especially as we still need to free() the stolen buffer. Instead, I decided to
update the comment in v2.
>> 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
ACK
>> +
>> +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.
ACK
>> +
>> +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'.
ACK
>> +
>> +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.
ACK
>> +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.
ACK
>> +
>> +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.
Not sure why, but previously I had dp port numbers change on me. I did another
100x run, and they are stable now, so I removed the -E.
>> +
>> +OVS_VSWITCHD_STOP
>> +AT_CLEANUP
>> +
>
> No need for the extra line.
ACK
>>
>> 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
