On Wed, Sep 2, 2026 at 4:33 PM Naveen Yerramneni < [email protected]> wrote:
> > > > On 2 Sep 2026, at 6:51 PM, Ales Musil <[email protected]> wrote: > > > > CAUTION: External Email > > > > > > On Wed, Sep 2, 2026 at 12:56 PM Naveen Yerramneni < > [email protected]> wrote: > > > > > >> 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. > > > > CoPP is directly proportional to packet-out in most of the > > cases. But that wasn't the original problem description, > > packet-in storm can be managed by CoPP. ovs-vswitchd not > > draining for whatever reason is a different problem entirely. > > In that case there is bigger problem, and sure limiting the > > queue might help to mitigate that, but it would have to apply > > to all packets that we send out of pinctrl, not just some > > chosen ones. > > > > > Hi Ales, > Hi Naveen, > > The original issue we hit was with ARP. CoPP would help in that > case. > > While analysing the issue we thought it is better to avoid unbounded > queues whether CoPP is enabled or not. The patch only limited > to packets sent in response to PACKET_IN. The goal is to restrict > all packet types. I think a bounded queue is a useful extra > safety layer. > > Shall I send a v2 that includes all packet types and makes the > limit configurable? > I don't think we should add yet another config option at the moment, as I don't see it as that big of deal. You can still monitor ovs-vswitchd or ovn to figure out if ovs-vswitchd is stuck. Of course I would love to hear the opinions of other maintainers. > > Thanks, > Naveen > Regards, Ales > > > > > > > >> > >> 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]]] [mail.openvswitch.org [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
