On Thu, Sep 3, 2026 at 7:31 AM Ales Musil <[email protected]> wrote:
>
>
>
> 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.
Let's say we add this config knob in OVN to limit the queue. What is the
behaviour of ovs-vswitchd in this case ? Will it keep sending the
packet-ins to the controller if CoPP is not enabled ?
Sorry if this is already answered above.
Thanks
Numan
>
>>
>>
>> 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
