> On 2 Sep 2026, at 2:31 PM, Ales Musil <[email protected]> wrote:
>
> CAUTION: External Email
>
>
> On Wed, Sep 2, 2026 at 10:46 AM Naveen Yerramneni
> <[email protected]> wrote:
>
>
> > On 2 Sep 2026, at 12:53 PM, Ales Musil <[email protected]> wrote:
> >
> > CAUTION: External Email
> >
> >
> > On Wed, Sep 2, 2026 at 9:03 AM Naveen Yerramneni
> > <[email protected]> wrote:
> >
> >
> > > 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.
> >
> > I don't think that should be the case, even if we add another config,
> > you will end up in the same situation as with CoPP. You will have
> > config that can protect you, which is disabled by default so it doesn't
> > help without configuring it first. Also, since this is a new feature, it
> > would mean 27.03, while CoPP has been available for a while. For
> > CoPP there is no upgrade needed or anything, just configuration.
>
> The proposal is to enable the pinctrl TX queue limit by default
> (8192). I think that is a reasonable default for the majority of
> deployments. A new Open_vSwitch:external_ids option would only let the
> user change that value, similar to OVS controller-queue-size
> (default 100). So the protection does not require extra configuration
> or a CMS change.
>
> But that is problematic, we cannot add a default restriction to something
> that wasn't restricted before. This applies anywhere in general, while
> 8k might be reasonable we can't be certain it indeed is for every system.
> What if ovn runs on small system? We might run out of memory anyway.
> Or, on the opposite end, for a large system, we would artificially limit the
> throughput. That's why it has to be opt-in, with the default remaining what
> it was before the change.
>
>
> I think we should avoid unbounded packet queues in general to avoid
> unbounded memory use.
>
> This is why we have CoPP.
CoPP controls the ingress PACKET_IN rate. If the rconn unix socket
is not draining for any reason, PACKET_OUT still piles up on
rconn.txq on the controller side.
>
> Thanks,
> Naveen
>
>
> Regards,
> Ales
Thanks,
Naveen
>
> >
> > 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
> >
> >
> > Regards,
> > Ales
> >
> > > 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] [mail.openvswitch.org [mail.openvswitch.org]]
> > > [mail.openvswitch.org [mail.openvswitch.org] [mail.openvswitch.org
> > > [mail.openvswitch.org]]]
> > >
> > >
> > > Regards,
> > > Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
