[ovs-dev] [PATCH v3] vswitchd: Make packet-in controller queue size configurable

Dumitru Ceara dceara at redhat.com
Tue Sep 24 07:22:15 UTC 2019


On Tue, Sep 24, 2019 at 1:39 AM Ben Pfaff <blp at ovn.org> wrote:
>
> Sorry about that.  Applied, thanks!

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