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

Dumitru Ceara dceara at redhat.com
Wed Jul 31 14:40:23 UTC 2019


On Wed, Jul 31, 2019 at 10:10 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> 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

v2 posted at: https://patchwork.ozlabs.org/patch/1139794/

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