> On 2 Sep 2026, at 11:55 AM, Ales Musil <[email protected]> wrote:
>
> CAUTION: External Email
>
>
> On Wed, Aug 26, 2026 at 7:16 PM Naveen Yerramneni
> <[email protected]> wrote:
> pinctrl enqueues PACKET_OUT and NXT_RESUME messages on rconn.txq with
> no limit. A PACKET_IN storm (ARP, ND, etc.) could grow the queue
> without bound and use a large amount of memory.
>
> Limit PACKET_IN driven PACKET_OUT and NXT_RESUME messages to 8192.
> Overflows are counted by the pinctrl_drop_rconn_overflow coverage
> counter. The limit does not apply to OpenFlow session control messages,
> MAC-binding buffered resumes, health-check probes, BFD and other
> controller originated packets.
>
> Acked-by: Aditya Mehakare <[email protected]>
> Assisted-by: Cursor Grok 4.6, Cursor
> Signed-off-by: Naveen Yerramneni <[email protected]>
> ---
> Hi Naveen,
Hi Ales,
>
> thank you for the patch. I don't think this is the right approach to
> solve any issues described above. This puts hard limit on the queue
> that cannot be controlled in any way by the user. Also all of the
> actions except for handle_dhcpv6_reply, which we could easily fix,
> have CoPP meter associated with it. So you can avoid the mentioned
> issues with proper CoPP configuration.
>
> In general if any pinctrl action can cause control plane issues we should
> have CoPP for it, if not we should add one.
Thanks for the review!
I agree CoPP helps in this case.
I still think the pinctrl send queue should be bounded, whether or not
CoPP is enabled. CoPP is optional and off by default. If an ARP
flood to a non-existent IP happens with CoPP unset, pinctrl can
enqueue packets without limit and ovn-controller can run out of memory.
We can make this queue size configurable through new option
in Open_vSwitch table (external_ids column).
OVS is giving similar control through controller-queue-size option.
Thanks,
Naveen
> controller/pinctrl.c | 48 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 216831e6e..ed903fc90 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -16,6 +16,8 @@
>
> #include <config.h>
>
> +#include <errno.h>
> +
> #include "pinctrl.h"
>
> #include "coverage.h"
> @@ -172,6 +174,9 @@ static struct seq *pinctrl_handler_seq;
> static struct seq *pinctrl_main_seq;
> static uint64_t main_seq;
>
> +/* Limit of tx packets can be queued on rconn.txq. */
> +#define PINCTRL_QUEUE_TX_PKT_LIMIT 8192
> +
> #define ARP_ND_DEF_MAX_TIMEOUT 16000
>
> static long long int arp_nd_max_timeout = ARP_ND_DEF_MAX_TIMEOUT;
> @@ -182,6 +187,8 @@ static void *pinctrl_handler(void *arg);
> struct pinctrl {
> /* OpenFlow connection to the switch. */
> struct rconn *swconn;
> + /* Counts tx packets queued on swconn. */
> + struct rconn_packet_counter *tx_pending_counter;
> pthread_t pinctrl_thread;
> /* Latch to destroy the 'pinctrl_thread' */
> struct latch pinctrl_thread_exit;
> @@ -397,6 +404,7 @@ COVERAGE_DEFINE(pinctrl_ring_full_put_fdb);
> COVERAGE_DEFINE(pinctrl_drop_buffered_packets_map);
> COVERAGE_DEFINE(pinctrl_drop_controller_event);
> COVERAGE_DEFINE(pinctrl_drop_put_vport_binding);
> +COVERAGE_DEFINE(pinctrl_drop_rconn_overflow);
> COVERAGE_DEFINE(pinctrl_notify_main_thread);
> COVERAGE_DEFINE(pinctrl_notify_handler_thread);
> COVERAGE_DEFINE(pinctrl_total_pin_pkts);
> @@ -580,6 +588,7 @@ pinctrl_init(void)
> bfd_monitor_init();
> init_fdb_entries();
> pinctrl.swconn = rconn_create(0, 0, DSCP_DEFAULT, 1 << OFP15_VERSION);
> + pinctrl.tx_pending_counter = rconn_packet_counter_create();
> pinctrl.mac_binding_can_timestamp = false;
> pinctrl_handler_seq = seq_create();
> pinctrl_main_seq = seq_create();
> @@ -600,6 +609,15 @@ queue_msg(struct rconn *swconn, struct ofpbuf *msg)
> return xid;
> }
>
> +static void
> +queue_msg_with_limit(struct rconn *swconn, struct ofpbuf *msg)
> +{
> + if (rconn_send_with_limit(swconn, msg, pinctrl.tx_pending_counter,
> + PINCTRL_QUEUE_TX_PKT_LIMIT) == EAGAIN) {
> + COVERAGE_INC(pinctrl_drop_rconn_overflow);
> + }
> +}
> +
> /* Sets up 'swconn', a newly (re)connected connection to a switch. */
> static void
> pinctrl_setup(struct rconn *swconn)
> @@ -639,7 +657,7 @@ enqueue_packet(struct rconn *swconn, enum ofp_version
> version,
>
> match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER);
> enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
> - queue_msg(swconn, ofputil_encode_packet_out(&po, proto));
> + queue_msg_with_limit(swconn, ofputil_encode_packet_out(&po, proto));
> }
>
> static void
> @@ -751,7 +769,7 @@ pinctrl_forward_pkt(struct rconn *swconn, int64_t dp_key,
> };
> match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER);
> enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
> - queue_msg(swconn, ofputil_encode_packet_out(&po, proto));
> + queue_msg_with_limit(swconn, ofputil_encode_packet_out(&po, proto));
> ofpbuf_uninit(&ofpacts);
> }
>
> @@ -1045,7 +1063,7 @@ pinctrl_parse_dhcpv6_advt(struct rconn *swconn, const
> struct flow *ip_flow,
> };
> match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER);
> enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
> - queue_msg(swconn, ofputil_encode_packet_out(&po, proto));
> + queue_msg_with_limit(swconn, ofputil_encode_packet_out(&po, proto));
> dp_packet_uninit(&packet);
> ofpbuf_uninit(&ofpacts);
>
> @@ -2382,7 +2400,8 @@ exit:
> sv.u8_val = success;
> mf_write_subfield(&dst, &sv, &pin->flow_metadata);
> }
> - queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto));
> + queue_msg_with_limit(swconn,
> + ofputil_encode_resume(pin, continuation, proto));
> if (pkt_out_ptr) {
> dp_packet_uninit(pkt_out_ptr);
> }
> @@ -2594,7 +2613,8 @@ exit:
> sv.u8_val = success;
> mf_write_subfield(&dst, &sv, &pin->flow_metadata);
> }
> - queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto));
> + queue_msg_with_limit(swconn,
> + ofputil_encode_resume(pin, continuation, proto));
> if (pkt_out_ptr) {
> dp_packet_uninit(pkt_out_ptr);
> }
> @@ -2934,7 +2954,8 @@ exit:
> sv.u8_val = success;
> mf_write_subfield(&dst, &sv, &pin->flow_metadata);
> }
> - queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto));
> + queue_msg_with_limit(swconn,
> + ofputil_encode_resume(pin, continuation, proto));
> if (pkt_out_ptr) {
> dp_packet_uninit(pkt_out_ptr);
> }
> @@ -3360,7 +3381,8 @@ exit:
> sv.u8_val = success;
> mf_write_subfield(&dst, &sv, &pin->flow_metadata);
> }
> - queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto));
> + queue_msg_with_limit(swconn,
> + ofputil_encode_resume(pin, continuation, proto));
> dp_packet_uninit(pkt_out_ptr);
> }
>
> @@ -3740,7 +3762,8 @@ exit:
> set_from_ctrl_flag_in_pkt_metadata(pin);
>
> }
> - queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto));
> + queue_msg_with_limit(swconn,
> + ofputil_encode_resume(pin, continuation, proto));
> dp_packet_uninit(pkt_out_ptr);
> }
>
> @@ -4781,6 +4804,7 @@ pinctrl_destroy(void)
> pthread_join(pinctrl.pinctrl_thread, NULL);
> latch_destroy(&pinctrl.pinctrl_thread_exit);
> rconn_destroy(pinctrl.swconn);
> + rconn_packet_counter_destroy(pinctrl.tx_pending_counter);
> destroy_send_arps_nds();
> destroy_ipv6_ras();
> destroy_ipv6_prefixd();
> @@ -6691,7 +6715,8 @@ exit:
> sv.u8_val = success;
> mf_write_subfield(&dst, &sv, &pin->flow_metadata);
> }
> - queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto));
> + queue_msg_with_limit(swconn,
> + ofputil_encode_resume(pin, continuation, proto));
> dp_packet_uninit(pkt_out_ptr);
> }
>
> @@ -6804,7 +6829,8 @@ pinctrl_handle_put_icmp4_inner_ip4_src(struct rconn
> *swconn,
> pin->packet_len = dp_packet_size(pkt_out);
>
> exit:
> - queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto));
> + queue_msg_with_limit(swconn,
> + ofputil_encode_resume(pin, continuation, proto));
> if (pkt_out) {
> dp_packet_delete(pkt_out);
> }
> @@ -9029,7 +9055,7 @@ pinctrl_split_buf_action_handler(struct rconn *swconn,
> struct dp_packet *pkt,
> match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER);
> enum ofp_version version = rconn_get_version(swconn);
> enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
> - queue_msg(swconn, ofputil_encode_packet_out(&po, proto));
> + queue_msg_with_limit(swconn, ofputil_encode_packet_out(&po, proto));
>
> ofpbuf_uninit(&ofpacts);
> }
> --
> 2.43.5
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev [mail.openvswitch.org]
>
>
> Regards,
> Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
