On 15 Jul 2025, at 14:57, Ilya Maximets wrote:
> On 7/15/25 11:13 AM, 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. >> >> Reportde-at: >> https://patchwork.ozlabs.org/project/openvswitch/patch/20250506022337.3242-1-danieldin...@gmail.com/ >> Fixes: 076caa2fb077 ("ofproto: Meter translation.") >> Signed-off-by: Eelco Chaudron <echau...@redhat.com> >> >> --- >> v2: - Removed redundant extra newlines. >> - Update comment to reflect reality >> - Use the add_of_ports MACRO >> - Use 'ovs-ofctl compose-packet --bare' for packet construction >> - Fix some grep commands. >> --- >> lib/dpif.c | 65 +++++++++++++++++-------------------------- >> tests/ofproto-dpif.at | 36 ++++++++++++++++++++++++ >> 2 files changed, 61 insertions(+), 40 deletions(-) > > Thanks, Eelco. This version seems fine overall, see a couple nits below. Thanks, I’ll sent a quick v3 fixing all your nits below. Cheers, Eelco >> >> diff --git a/lib/dpif.c b/lib/dpif.c >> index a064f717f..3d0d366f1 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,21 @@ 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) { >> + 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 +1246,9 @@ 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; >> - } >> + /* Re-initialize the 'aux->meter_actions' ofpbuf as it could have >> been >> + * used for sending the additional meter and/or tunnel actions. */ >> + ofpbuf_reinit(&aux->meter_actions, 0); > > Should we maybe use ofpbuf_clear() instead? It doesn't seem to > be necessary to reallocate the buffer. Ack, this would be nicer. >> 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 > > Trailing comma is missing. > >> + }; >> 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..770337361 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -6621,6 +6621,42 @@ 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 >> + >> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-meter br0 \ >> + 'meter=1 pktps burst stats bands=type=drop rate=100 burst_size=100']) >> + >> +add_of_ports --pcap br0 1 2 >> + >> +AT_CHECK([ovs-appctl vlog/set dpif:dbg]) >> + >> +eth="eth_src=00:00:00:00:00:ec,eth_dst=ff:ff:ff:ff:ff:ff,arp" >> +arp="arp_tpa=192.168.0.1,arp_spa=192.168.0.100,arp_op=1" >> +packet=$(ovs-ofctl compose-packet --bare "$eth,$arp") >> + >> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 dnl > > 'dnl' here, but '\' in the command below. Might be better to just use > '\' in all cases. Modern autotest works fine with line continuations. > Even better than with dnl, as dnl sometimes ends up with the rest of > the command not showing up in the log. > >> + >> "table=1,eth_type=0x0806,actions=meter:1,clone(set_field:2->arp_op,output(port=p2,max_len=128)),output:p1"]) >> + >> +AT_CHECK([ovs-ofctl -O OpenFlow15 packet-out br0 CONTROLLER \ >> + "meter:1,resubmit(,1)" "$packet"]) >> + >> +dnl Verify the orinigal packet is received on port 1. >> +OVS_WAIT_UNTIL([test `ovs-pcap p1-tx.pcap | grep -c "$packet"` -eq 1]) > > May be better to use $() syntax instead of ``, since you're already > using $() above. > >> + >> +dnl Verify the modified packet is received on port 2. >> +arp="arp_tpa=192.168.0.1,arp_spa=192.168.0.100,arp_op=2" >> +packet=$(ovs-ofctl compose-packet --bare "$eth,$arp") >> +OVS_WAIT_UNTIL([test `ovs-pcap p2-tx.pcap | grep -c "$packet"` -eq 1]) > > $() > >> + >> +dnl Make sure all meters, and outputs datapath actions get executed. >> +OVS_WAIT_UNTIL([grep "sub-execute meter(0),meter(0),2 on packet arp" dnl >> + ovs-vswitchd.log]) > > dnl vs '\' > >> +OVS_WAIT_UNTIL([grep "sub-execute 1 on packet arp" ovs-vswitchd.log]) >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> >> 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
