| Message ID | 1564482775-13392-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] vswitchd: Make packet-in controller queue size configurable | expand |
Hi Dumitru, The C code looks good to me, but I had some findings in the schema update and documentation below. Is this something that could have a test added? 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@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 > "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. > <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 >
On Tue, Jul 30, 2019 at 8:49 PM Mark Michelson <mmichels@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@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 > > >
On Wed, Jul 31, 2019 at 10:10 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On Tue, Jul 30, 2019 at 8:49 PM Mark Michelson <mmichels@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@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 > > > > >
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", "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> + <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
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@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(-)