[ovs-dev] [PATCH v3] vswitchd: Make packet-in controller queue size configurable
Ben Pfaff
blp at ovn.org
Mon Sep 23 22:48:52 UTC 2019
Sorry about that. Applied, thanks!
On Mon, Sep 16, 2019 at 12:24:24PM +0200, Dumitru Ceara wrote:
> Hi,
>
> Just a reminder, Mark has acked this change a while ago but it didn't
> get pushed yet.
>
> Thanks,
> Dumitru
>
> On Fri, Aug 9, 2019 at 5:08 PM Mark Michelson <mmichels at redhat.com> wrote:
> >
> > Acked-by: Mark Michelson <mmichels at redhat.com>
> >
> > On 8/2/19 4:29 AM, Dumitru Ceara wrote:
> > > The ofconn packet-in queue for packets that can't be immediately sent
> > > on the rconn connection was limited to 100 packets (hardcoded value).
> > > While increasing this limit is usually not recommended as it might
> > > create buffer bloat and increase latency, in scaled scenarios it is
> > > useful if the administrator (or CMS) can adjust the queue size.
> > >
> > > One such situation was noticed while performing scale testing of the
> > > OVN IGMP functionality: triggering ~200 simultaneous IGMP reports
> > > was causing tail drops on the packet-in queue towards ovn-controller.
> > >
> > > This commit adds the possibility to configure the queue size for:
> > > - management controller (br-int.mgmt): through the
> > > other_config:controller-queue-size column of the Bridge table. This
> > > value is limited to 512 as large queues definitely affect latency. If
> > > not present the default value of 100 is used. This is done in order to
> > > maintain the same default behavior as before the commit.
> > > - other controllers: through the controller_queue_size column of the
> > > Controller table. This value is also limited to 512. If not present
> > > the code uses the Bridge:other_config:controller-queue-size
> > > configuration.
> > >
> > > Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> > >
> > > ---
> > > v3: Remove trailing whitespace.
> > > v2: Address reviewer comments (increase vswitch.vsschema version &
> > > reword docs).
> > > ---
> > > ofproto/connmgr.c | 6 +++++-
> > > ofproto/ofproto.h | 1 +
> > > vswitchd/bridge.c | 26 ++++++++++++++++++++++++++
> > > vswitchd/vswitch.ovsschema | 9 +++++++--
> > > vswitchd/vswitch.xml | 21 +++++++++++++++++++++
> > > 5 files changed, 60 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> > > index d975a53..51d656c 100644
> > > --- a/ofproto/connmgr.c
> > > +++ b/ofproto/connmgr.c
> > > @@ -74,6 +74,7 @@ struct ofconn {
> > > enum ofputil_packet_in_format packet_in_format;
> > >
> > > /* OFPT_PACKET_IN related data. */
> > > + int packet_in_queue_size;
> > > struct rconn_packet_counter *packet_in_counter; /* # queued on 'rconn'. */
> > > #define N_SCHEDULERS 2
> > > struct pinsched *schedulers[N_SCHEDULERS];
> > > @@ -1176,6 +1177,7 @@ ofconn_create(struct ofservice *ofservice, struct rconn *rconn,
> > > ofconn_set_protocol(ofconn, OFPUTIL_P_NONE);
> > > ofconn->packet_in_format = OFPUTIL_PACKET_IN_STD;
> > >
> > > + ofconn->packet_in_queue_size = settings->max_pktq_size;
> > > ofconn->packet_in_counter = rconn_packet_counter_create();
> > > ofconn->miss_send_len = (ofconn->type == OFCONN_PRIMARY
> > > ? OFP_DEFAULT_MISS_SEND_LEN
> > > @@ -1263,6 +1265,7 @@ ofconn_reconfigure(struct ofconn *ofconn, const struct ofproto_controller *c)
> > > rconn_set_probe_interval(ofconn->rconn, probe_interval);
> > >
> > > ofconn->band = c->band;
> > > + ofconn->packet_in_queue_size = c->max_pktq_size;
> > >
> > > ofconn_set_rate_limit(ofconn, c->rate_limit, c->burst_limit);
> > >
> > > @@ -1676,7 +1679,8 @@ do_send_packet_ins(struct ofconn *ofconn, struct ovs_list *txq)
> > >
> > > LIST_FOR_EACH_POP (pin, list_node, txq) {
> > > if (rconn_send_with_limit(ofconn->rconn, pin,
> > > - ofconn->packet_in_counter, 100) == EAGAIN) {
> > > + ofconn->packet_in_counter,
> > > + ofconn->packet_in_queue_size) == EAGAIN) {
> > > static struct vlog_rate_limit rll = VLOG_RATE_LIMIT_INIT(5, 5);
> > >
> > > VLOG_INFO_RL(&rll, "%s: dropping packet-in due to queue overflow",
> > > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > > index 6e4afff..b53ea03 100644
> > > --- a/ofproto/ofproto.h
> > > +++ b/ofproto/ofproto.h
> > > @@ -234,6 +234,7 @@ struct ofproto_controller {
> > > * be negotiated for a session. */
> > >
> > > /* OpenFlow packet-in rate-limiting. */
> > > + int max_pktq_size; /* Maximum number of packet-in to be queued. */
> > > int rate_limit; /* Max packet-in rate in packets per second. */
> > > int burst_limit; /* Limit on accumulating packet credits. */
> > >
> > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > > index 2976771..f9d17b7 100644
> > > --- a/vswitchd/bridge.c
> > > +++ b/vswitchd/bridge.c
> > > @@ -227,6 +227,11 @@ static struct if_notifier *ifnotifier;
> > > static struct seq *ifaces_changed;
> > > static uint64_t last_ifaces_changed;
> > >
> > > +/* Default/min/max packet-in queue sizes towards the controllers. */
> > > +#define BRIDGE_CONTROLLER_PACKET_QUEUE_DEFAULT_SIZE 100
> > > +#define BRIDGE_CONTROLLER_PACKET_QUEUE_MIN_SIZE 1
> > > +#define BRIDGE_CONTROLLER_PACKET_QUEUE_MAX_SIZE 512
> > > +
> > > static void add_del_bridges(const struct ovsrec_open_vswitch *);
> > > static void bridge_run__(void);
> > > static void bridge_create(const struct ovsrec_bridge *);
> > > @@ -1117,6 +1122,25 @@ bridge_get_allowed_versions(struct bridge *br)
> > > br->cfg->n_protocols);
> > > }
> > >
> > > +static int
> > > +bridge_get_controller_queue_size(struct bridge *br,
> > > + struct ovsrec_controller *c)
> > > +{
> > > + if (c && c->controller_queue_size) {
> > > + return *c->controller_queue_size;
> > > + }
> > > +
> > > + int queue_size = smap_get_int(&br->cfg->other_config,
> > > + "controller-queue-size",
> > > + BRIDGE_CONTROLLER_PACKET_QUEUE_DEFAULT_SIZE);
> > > + if (queue_size < BRIDGE_CONTROLLER_PACKET_QUEUE_MIN_SIZE ||
> > > + queue_size > BRIDGE_CONTROLLER_PACKET_QUEUE_MAX_SIZE) {
> > > + return BRIDGE_CONTROLLER_PACKET_QUEUE_DEFAULT_SIZE;
> > > + }
> > > +
> > > + return queue_size;
> > > +}
> > > +
> > > /* Set NetFlow configuration on 'br'. */
> > > static void
> > > bridge_configure_netflow(struct bridge *br)
> > > @@ -3605,6 +3629,7 @@ bridge_configure_remotes(struct bridge *br,
> > > .band = OFPROTO_OUT_OF_BAND,
> > > .enable_async_msgs = true,
> > > .allowed_versions = bridge_get_allowed_versions(br),
> > > + .max_pktq_size = bridge_get_controller_queue_size(br, NULL),
> > > };
> > > shash_add_nocopy(
> > > &ocs, xasprintf("punix:%s/%s.mgmt", ovs_rundir(), br->name), oc);
> > > @@ -3680,6 +3705,7 @@ bridge_configure_remotes(struct bridge *br,
> > > .enable_async_msgs = (!c->enable_async_messages
> > > || *c->enable_async_messages),
> > > .allowed_versions = bridge_get_allowed_versions(br),
> > > + .max_pktq_size = bridge_get_controller_queue_size(br, c),
> > > .rate_limit = (c->controller_rate_limit
> > > ? *c->controller_rate_limit : 0),
> > > .burst_limit = (c->controller_burst_limit
> > > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> > > index f7c6eb8..4f7ba59 100644
> > > --- a/vswitchd/vswitch.ovsschema
> > > +++ b/vswitchd/vswitch.ovsschema
> > > @@ -1,6 +1,6 @@
> > > {"name": "Open_vSwitch",
> > > - "version": "8.0.0",
> > > - "cksum": "3962141869 23978",
> > > + "version": "8.1.0",
> > > + "cksum": "3492192244 24186",
> > > "tables": {
> > > "Open_vSwitch": {
> > > "columns": {
> > > @@ -575,6 +575,11 @@
> > > "enable_async_messages": {
> > > "type": {"key": {"type": "boolean"},
> > > "min": 0, "max": 1}},
> > > + "controller_queue_size": {
> > > + "type": {"key": {"type": "integer",
> > > + "minInteger": 1,
> > > + "maxInteger": 512},
> > > + "min": 0, "max": 1}},
> > > "controller_rate_limit": {
> > > "type": {"key": {"type": "integer",
> > > "minInteger": 100},
> > > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > > index 027aee2..d05d423 100644
> > > --- a/vswitchd/vswitch.xml
> > > +++ b/vswitchd/vswitch.xml
> > > @@ -1268,6 +1268,15 @@
> > > ID, the default queue is used instead.
> > > </column>
> > >
> > > + <column name="other_config" key="controller-queue-size"
> > > + type='{"type": "integer", "minInteger": 1, "maxInteger": 512}'>
> > > + This sets the maximum size of the queue of packets that need to be
> > > + sent to the OpenFlow management controller. The value must be less
> > > + than 512. If not specified the queue size is limited to 100 packets
> > > + by default. Note: increasing the queue size might have a negative
> > > + impact on latency.
> > > + </column>
> > > +
> > > <column name="protocols">
> > > List of OpenFlow protocols that may be used when negotiating a
> > > connection with a controller. OpenFlow 1.0, 1.1, 1.2, 1.3, 1.4, and
> > > @@ -5003,6 +5012,18 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
> > > table="Interface"/> table for ingress policing configuration.
> > > </p>
> > >
> > > + <column name="controller_queue_size">
> > > + <p>
> > > + This sets the maximum size of the queue of packets that need to be
> > > + sent to this OpenFlow controller. The value must be less than 512.
> > > + If not specified the queue size is limited to the value set for
> > > + the management controller in <ref table="Bridge"
> > > + column="other_config" key="controller-queue-size"/> if present or
> > > + 100 packets by default. Note: increasing the queue size might
> > > + have a negative impact on latency.
> > > + </p>
> > > + </column>
> > > +
> > > <column name="controller_rate_limit">
> > > <p>
> > > The maximum rate at which the switch will forward packets to the
> > >
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list