Re: [ovs-dev] [PATCH v3 ovn] controller: reconfigure ovs meters for ovn meters updates
Hi Lorenzo,
I think this is an improvement over the previous version, but there are
still issues that need addressing.
The problem with adding meter incremental processing is that there
doesn't exist any unconditional processing of the SB meter data. In
other words, there is no place in the code that loops over all SB meters
and programs those into OVS. Instead, our lflow processing code (which
is incrementally processed) determines which meters are referenced by
lflows and then later uses this to determine which meters are "relevant"
and only reads the SB meters that are referred to by lflows.
In each iteration of the patch, adding incremental processing to SB
meters has had oddities since we're not replacing an existing stateless
SB meter process. At a high level, the oddities I see here are:
1) Rather than using engine node data to store SB meter data, this
change adds priv_data and priv_size to the existing extend_table_info
structure.
2) The en_meter_run() function doesn't do anything.
3) The meter_sb_meter_handler() function only handles updated meters,
not new or deleted meters.
In all of these cases, the reason is that we are still relying on lflow
processing to do a lot of the heavy lifting (i.e. discovering new
meters, deleting removed meters, etc). The new engine node is tightly
coupled with the lflow engine node and is only designed to patch some
shortcomings, rather than having a fully-functioning incremental
processing node for meters. This could fall apart spectacularly if lflow
processing changes.
I think your previous attempts to make a separate engine to handle meter
updates was an attempt to decouple the meter processing from the lflow
processing, but unfortunately the coupling was still present in those
versions since lflow handling was still creating the extend_table_info
structures and needed to add their references to the extend_table_info.
I think that in order to add incremental processing for SB meters, it's
a two-step process.
Step 1: Write non-incremental code that centers meter programming around
the SB meter database instead of based on lflows that refer to meters.
Step 2: Take the code written in step 1 and make it incremental.
This patch is trying to do step 2 without step 1. The thing is, to fix
the issue, you could submit a patch that just does step 1 without step
2, and it may be just fine, depending on how common it is for meters to
change.
I hope what I'm saying makes sense. I have some additional comments down
below
On 2/10/22 18:11, Lorenzo Bianconi wrote:
At the moment ovs meters are reconfigured by ovn just when a
new a meter is allocated while updates for an already allocated meter are
ignored. This issue can be easily verified with the following reproducer:
$ovn-nbctl meter-add meter0 drop 10 pktps
$ovn-nbctl --log --meter=meter0 acl-add sw0 to-lport 1000 'tcp.dst == 80' drop
$ovn-nbctl --may-exist meter-add meter0 drop 20 pktps
$ovs-ofctl -O OpenFlow15 dump-meters br-int
Fix the issue reconfiguring ovs meters even for ovn meters updates.
Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
Signed-off-by: Lorenzo Bianconi
---
Changes since v2:
- move meter ip into lflow ip
- add new test in ovn-performance.at
- manage metering ip full-recompute
Changes since v1:
- add IP refactor to the series
- rebase on top of ovn master
---
controller/ofctrl.c | 101
controller/ofctrl.h | 3 ++
controller/ovn-controller.c | 57 +++-
lib/extend-table.c | 6 +++
lib/extend-table.h | 2 +
tests/ovn-performance.at| 15 ++
tests/ovn.at| 51 ++
tests/system-ovn.at | 17 ++
8 files changed, 230 insertions(+), 22 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 08fcfed8b..c001c8ad1 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -344,8 +344,6 @@ static enum mf_field_id mff_ovn_geneve;
* is restarted, even if there is no change in the desired flow table. */
static bool need_reinstall_flows;
-static ovs_be32 queue_msg(struct ofpbuf *);
-
static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
static struct ofpbuf *encode_group_mod(const struct ofputil_group_mod *);
@@ -797,7 +795,7 @@ ofctrl_get_cur_cfg(void)
return cur_cfg;
}
-static ovs_be32
+ovs_be32
queue_msg(struct ofpbuf *msg)
{
const struct ofp_header *oh = msg->data;
@@ -1802,26 +1800,12 @@ add_meter_string(struct ovn_extend_table_info
*m_desired,
}
static void
-add_meter(struct ovn_extend_table_info *m_desired,
- const struct sbrec_meter_table *meter_table,
- struct ovs_list *msgs)
+meter_create_msg(const struct sbrec_meter *sb_meter,
+ struct ovs_list *msgs, int cmd, int id)
{
-const struct sbrec_meter *sb_meter;
-SBREC_METER_TABLE_FOR_EACH (sb_meter,
Re: [ovs-dev] [PATCH v3 ovn] controller: reconfigure ovs meters for ovn meters updates
Bleep bloop. Greetings Lorenzo Bianconi, I am a robot and I have tried out
your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
ERROR: Inappropriate bracing around statement
#100 FILE: controller/ofctrl.c:1853:
for (j = 0; j < m_installed->priv_size / sizeof *band; j++) {
WARNING: Line is 81 characters long (recommended limit is 79)
#117 FILE: controller/ofctrl.c:1870:
band = (struct sbrec_meter_band
*)m_installed->priv_data;
WARNING: Line is 81 characters long (recommended limit is 79)
#335 FILE: lib/extend-table.h:56:
void *priv_data; /* Pointer to private data (e.g. meter-bands for meters).
*/
Lines checked: 457, Warnings: 2, Errors: 1
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 ovn] controller: reconfigure ovs meters for ovn meters updates
At the moment ovs meters are reconfigured by ovn just when a
new a meter is allocated while updates for an already allocated meter are
ignored. This issue can be easily verified with the following reproducer:
$ovn-nbctl meter-add meter0 drop 10 pktps
$ovn-nbctl --log --meter=meter0 acl-add sw0 to-lport 1000 'tcp.dst == 80' drop
$ovn-nbctl --may-exist meter-add meter0 drop 20 pktps
$ovs-ofctl -O OpenFlow15 dump-meters br-int
Fix the issue reconfiguring ovs meters even for ovn meters updates.
Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
Signed-off-by: Lorenzo Bianconi
---
Changes since v2:
- move meter ip into lflow ip
- add new test in ovn-performance.at
- manage metering ip full-recompute
Changes since v1:
- add IP refactor to the series
- rebase on top of ovn master
---
controller/ofctrl.c | 101
controller/ofctrl.h | 3 ++
controller/ovn-controller.c | 57 +++-
lib/extend-table.c | 6 +++
lib/extend-table.h | 2 +
tests/ovn-performance.at| 15 ++
tests/ovn.at| 51 ++
tests/system-ovn.at | 17 ++
8 files changed, 230 insertions(+), 22 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 08fcfed8b..c001c8ad1 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -344,8 +344,6 @@ static enum mf_field_id mff_ovn_geneve;
* is restarted, even if there is no change in the desired flow table. */
static bool need_reinstall_flows;
-static ovs_be32 queue_msg(struct ofpbuf *);
-
static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
static struct ofpbuf *encode_group_mod(const struct ofputil_group_mod *);
@@ -797,7 +795,7 @@ ofctrl_get_cur_cfg(void)
return cur_cfg;
}
-static ovs_be32
+ovs_be32
queue_msg(struct ofpbuf *msg)
{
const struct ofp_header *oh = msg->data;
@@ -1802,26 +1800,12 @@ add_meter_string(struct ovn_extend_table_info
*m_desired,
}
static void
-add_meter(struct ovn_extend_table_info *m_desired,
- const struct sbrec_meter_table *meter_table,
- struct ovs_list *msgs)
+meter_create_msg(const struct sbrec_meter *sb_meter,
+ struct ovs_list *msgs, int cmd, int id)
{
-const struct sbrec_meter *sb_meter;
-SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
-if (!strcmp(m_desired->name, sb_meter->name)) {
-break;
-}
-}
-
-if (!sb_meter) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_ERR_RL(, "could not find meter named \"%s\"", m_desired->name);
-return;
-}
-
struct ofputil_meter_mod mm;
-mm.command = OFPMC13_ADD;
-mm.meter.meter_id = m_desired->table_id;
+mm.command = cmd;
+mm.meter.meter_id = id;
mm.meter.flags = OFPMF13_STATS;
if (!strcmp(sb_meter->unit, "pktps")) {
@@ -1854,6 +1838,76 @@ add_meter(struct ovn_extend_table_info *m_desired,
free(mm.meter.bands);
}
+void
+meter_update(const struct sbrec_meter *sb_meter, struct ovs_list *msgs)
+{
+struct ovn_extend_table_info *m_installed, *next_meter;
+HMAP_FOR_EACH_SAFE (m_installed, next_meter, hmap_node,
+>existing) {
+if (!strcmp(m_installed->name, sb_meter->name)) {
+struct sbrec_meter_band *band;
+
+for (int i = 0; i < sb_meter->n_bands; i++) {
+int j;
+
+for (j = 0; j < m_installed->priv_size / sizeof *band; j++) {
+band =
+(struct sbrec_meter_band *)m_installed->priv_data + j;
+if (band->rate == sb_meter->bands[i]->rate &&
+band->burst_size == sb_meter->bands[i]->burst_size) {
+break;
+}
+}
+
+if (j == m_installed->priv_size / sizeof *band) {
+meter_create_msg(sb_meter, msgs, OFPMC13_MODIFY,
+ m_installed->table_id);
+/* Recreate meter-bands cache. */
+free(m_installed->priv_data);
+m_installed->priv_size = sb_meter->n_bands * sizeof *band;
+m_installed->priv_data = xmalloc(m_installed->priv_size);
+for (i = 0; i < sb_meter->n_bands; i++) {
+band = (struct sbrec_meter_band
*)m_installed->priv_data;
+memcpy(band + i, sb_meter->bands[i], sizeof *band);
+}
+return;
+}
+}
+}
+}
+}
+
+static void
+add_meter(struct ovn_extend_table_info *m_desired,
+ const struct sbrec_meter_table *meter_table,
+ struct ovs_list *msgs)
+{
+const struct sbrec_meter *sb_meter;
+SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
+if (!strcmp(m_desired->name, sb_meter->name)) {
+
