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

Dumitru Ceara dceara at redhat.com
Wed Jul 31 08:10:21 UTC 2019


On Tue, Jul 30, 2019 at 8:49 PM Mark Michelson <mmichels at redhat.com> wrote:
>
> Hi Dumitru,
>
> The C code looks good to me, but I had some findings in the schema
> update and documentation below.

Hi Mark,

Thanks for the review.

>
> Is this something that could have a test added?

To be honest I'm not so sure how we can test that the limit is
enforced properly in a deterministic way. We'd need to ensure that
packets are received by vswitchd in a burst such that the queue fills
up but that depends quite a lot on the machine we test on. Any
suggestions?

>
> On 7/30/19 6:32 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>
> > ---
> >   ofproto/connmgr.c          |  6 +++++-
> >   ofproto/ofproto.h          |  1 +
> >   vswitchd/bridge.c          | 26 ++++++++++++++++++++++++++
> >   vswitchd/vswitch.ovsschema |  7 ++++++-
> >   vswitchd/vswitch.xml       | 21 +++++++++++++++++++++
> >   5 files changed, 59 insertions(+), 2 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..17bbf52 100644
> > --- a/vswitchd/vswitch.ovsschema
> > +++ b/vswitchd/vswitch.ovsschema
> > @@ -1,6 +1,6 @@
> >   {"name": "Open_vSwitch",
> >    "version": "8.0.0",
> > - "cksum": "3962141869 23978",
> > + "cksum": "1630138495 24186",
>
> Adding a new column to the database should increase the minor version.
> So this should increase the version to 8.1.0

Right, i didn't know that, thanks for pointing it out.

>
> >    "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..abc0278 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 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>
> > +
>
> It's not clear from the documentation what the difference is between
> other_config:controller-queue-size and controller_queue_size. The only
> difference between the descriptions is how defaults are chosen. However,
> the actual difference in what the settings do is not made clear.

Well, they do the same thing but they have different scopes (they're
on different tables). The controller-queue-size in table Bridge sets
the max queue size for the management controller, one per bridge. Otoh
the "controller_queue_size" (I used "_" instead of "-" to keep the
style similar with the other columns) sets the max queue size for the
specific user defined controller. A bridge can have multiple
controllers attached to it.
I'll try to change the wording a bit to make it more clear and send a v2.

Thanks,
Dumitru

>
> >         <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 the OpenFlow controller. The value must be less than 512.
> > +           If not specified the queue size is limited to the value set 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
> >
>


More information about the dev mailing list