The migration code introduced a number of flow traversal macros. This series cleans up their interfaces a bit, and makes them more widely useful. David Gibson (3): flow: Remove unneeded index from foreach_* macros flow: Remove unneeded bound parameter from flow traversal macros flow: Clean up and generalise flow traversal macros flow.c | 72 +++++++++++++++++++--------------------------------- flow_table.h | 36 ++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 46 deletions(-) -- 2.48.1
The foreach macros are odd in that they take two loop counters: an integer
index, and a pointer to the flow. We nearly always want the latter, not
the former, and we can get the index from the pointer trivially when we
need it. So, rearrange the macros not to need the integer index.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
flow.c | 44 +++++++++++++++++++++-----------------------
1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/flow.c b/flow.c
index c68f6bb1..3fcdd9f2 100644
--- a/flow.c
+++ b/flow.c
@@ -53,30 +53,28 @@ const uint8_t flow_proto[] = {
static_assert(ARRAY_SIZE(flow_proto) == FLOW_NUM_TYPES,
"flow_proto[] doesn't match enum flow_type");
-#define foreach_flow(i, flow, bound) \
- for ((i) = 0, (flow) = &flowtab[(i)]; \
- (i) < (bound); \
- (i)++, (flow) = &flowtab[(i)]) \
+#define foreach_flow(flow, bound) \
+ for ((flow) = flowtab; FLOW_IDX(flow) < (bound); (flow)++) \
if ((flow)->f.state == FLOW_STATE_FREE) \
- (i) += (flow)->free.n - 1; \
+ (flow) += (flow)->free.n - 1; \
else
-#define foreach_active_flow(i, flow, bound) \
- foreach_flow((i), (flow), (bound)) \
+#define foreach_active_flow(flow, bound) \
+ foreach_flow((flow), (bound)) \
if ((flow)->f.state != FLOW_STATE_ACTIVE) \
/* NOLINTNEXTLINE(bugprone-branch-clone) */ \
continue; \
else
-#define foreach_tcp_flow(i, flow, bound) \
- foreach_active_flow((i), (flow), (bound)) \
+#define foreach_tcp_flow(flow, bound) \
+ foreach_active_flow((flow), (bound)) \
if ((flow)->f.type != FLOW_TCP) \
/* NOLINTNEXTLINE(bugprone-branch-clone) */ \
continue; \
else
-#define foreach_established_tcp_flow(i, flow, bound) \
- foreach_tcp_flow((i), (flow), (bound)) \
+#define foreach_established_tcp_flow(flow, bound) \
+ foreach_tcp_flow((flow), (bound)) \
if (!tcp_flow_is_established(&(flow)->tcp)) \
/* NOLINTNEXTLINE(bugprone-branch-clone) */ \
continue; \
@@ -918,11 +916,10 @@ static int flow_migrate_source_rollback(struct ctx *c, unsigned
max_flow,
int ret)
{
union flow *flow;
- unsigned i;
debug("...roll back migration");
- foreach_established_tcp_flow(i, flow, max_flow)
+ foreach_established_tcp_flow(flow, max_flow)
if (tcp_flow_repair_off(c, &flow->tcp))
die("Failed to roll back TCP_REPAIR mode");
@@ -942,10 +939,9 @@ static int flow_migrate_source_rollback(struct ctx *c, unsigned
max_flow,
static int flow_migrate_repair_all(struct ctx *c, bool enable)
{
union flow *flow;
- unsigned i;
int rc;
- foreach_established_tcp_flow(i, flow, FLOW_MAX) {
+ foreach_established_tcp_flow(flow, FLOW_MAX) {
if (enable)
rc = tcp_flow_repair_on(c, &flow->tcp);
else
@@ -954,14 +950,15 @@ static int flow_migrate_repair_all(struct ctx *c, bool enable)
if (rc) {
debug("Can't %s repair mode: %s",
enable ? "enable" : "disable", strerror_(-rc));
- return flow_migrate_source_rollback(c, i, rc);
+ return flow_migrate_source_rollback(c, FLOW_IDX(flow),
+ rc);
}
}
if ((rc = repair_flush(c))) {
debug("Can't %s repair mode: %s",
enable ? "enable" : "disable", strerror_(-rc));
- return flow_migrate_source_rollback(c, i, rc);
+ return flow_migrate_source_rollback(c, FLOW_IDX(flow), rc);
}
return 0;
@@ -1003,13 +1000,12 @@ int flow_migrate_source(struct ctx *c, const struct migrate_stage
*stage,
uint32_t count = 0;
bool first = true;
union flow *flow;
- unsigned i;
int rc;
(void)c;
(void)stage;
- foreach_established_tcp_flow(i, flow, FLOW_MAX)
+ foreach_established_tcp_flow(flow, FLOW_MAX)
count++;
count = htonl(count);
@@ -1028,10 +1024,11 @@ int flow_migrate_source(struct ctx *c, const struct migrate_stage
*stage,
* stream might now be inconsistent, and we might have closed listening
* TCP sockets, so just terminate.
*/
- foreach_established_tcp_flow(i, flow, FLOW_MAX) {
+ foreach_established_tcp_flow(flow, FLOW_MAX) {
rc = tcp_flow_migrate_source(fd, &flow->tcp);
if (rc) {
- err("Can't send data, flow %u: %s", i, strerror_(-rc));
+ err("Can't send data, flow %u: %s", FLOW_IDX(flow),
+ strerror_(-rc));
if (!first)
die("Inconsistent migration state, exiting");
@@ -1054,10 +1051,11 @@ int flow_migrate_source(struct ctx *c, const struct migrate_stage
*stage,
* failures but not if the stream might be inconsistent (reported here
* as EIO).
*/
- foreach_established_tcp_flow(i, flow, FLOW_MAX) {
+ foreach_established_tcp_flow(flow, FLOW_MAX) {
rc = tcp_flow_migrate_source_ext(fd, &flow->tcp);
if (rc) {
- err("Extended data for flow %u: %s", i, strerror_(-rc));
+ err("Extended data for flow %u: %s", FLOW_IDX(flow),
+ strerror_(-rc));
if (rc == -EIO)
die("Inconsistent migration state, exiting");
--
2.48.1
The foreach macros used to step through flows each take a 'bound' parameter
to only scan part of the flow table. Only one place actually passes a
bound different from FLOW_MAX. So we can simplify every other invocation
by having that one case manually handle the bound.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
flow.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/flow.c b/flow.c
index 3fcdd9f2..602fea79 100644
--- a/flow.c
+++ b/flow.c
@@ -53,28 +53,28 @@ const uint8_t flow_proto[] = {
static_assert(ARRAY_SIZE(flow_proto) == FLOW_NUM_TYPES,
"flow_proto[] doesn't match enum flow_type");
-#define foreach_flow(flow, bound) \
- for ((flow) = flowtab; FLOW_IDX(flow) < (bound); (flow)++) \
+#define foreach_flow(flow) \
+ for ((flow) = flowtab; FLOW_IDX(flow) < FLOW_MAX; (flow)++) \
if ((flow)->f.state == FLOW_STATE_FREE) \
(flow) += (flow)->free.n - 1; \
else
-#define foreach_active_flow(flow, bound) \
- foreach_flow((flow), (bound)) \
+#define foreach_active_flow(flow) \
+ foreach_flow((flow)) \
if ((flow)->f.state != FLOW_STATE_ACTIVE) \
/* NOLINTNEXTLINE(bugprone-branch-clone) */ \
continue; \
else
-#define foreach_tcp_flow(flow, bound) \
- foreach_active_flow((flow), (bound)) \
+#define foreach_tcp_flow(flow) \
+ foreach_active_flow((flow)) \
if ((flow)->f.type != FLOW_TCP) \
/* NOLINTNEXTLINE(bugprone-branch-clone) */ \
continue; \
else
-#define foreach_established_tcp_flow(flow, bound) \
- foreach_tcp_flow((flow), (bound)) \
+#define foreach_established_tcp_flow(flow) \
+ foreach_tcp_flow((flow)) \
if (!tcp_flow_is_established(&(flow)->tcp)) \
/* NOLINTNEXTLINE(bugprone-branch-clone) */ \
continue; \
@@ -907,21 +907,23 @@ void flow_defer_handler(const struct ctx *c, const struct timespec
*now)
/**
* flow_migrate_source_rollback() - Disable repair mode, return failure
* @c: Execution context
- * @max_flow: Maximum index of affected flows
+ * @bound: No need to roll back flow indices >= @bound
* @ret: Negative error code
*
* Return: @ret
*/
-static int flow_migrate_source_rollback(struct ctx *c, unsigned max_flow,
- int ret)
+static int flow_migrate_source_rollback(struct ctx *c, unsigned bound, int ret)
{
union flow *flow;
debug("...roll back migration");
- foreach_established_tcp_flow(flow, max_flow)
+ foreach_established_tcp_flow(flow) {
+ if (FLOW_IDX(flow) >= bound)
+ break;
if (tcp_flow_repair_off(c, &flow->tcp))
die("Failed to roll back TCP_REPAIR mode");
+ }
if (repair_flush(c))
die("Failed to roll back TCP_REPAIR mode");
@@ -941,7 +943,7 @@ static int flow_migrate_repair_all(struct ctx *c, bool enable)
union flow *flow;
int rc;
- foreach_established_tcp_flow(flow, FLOW_MAX) {
+ foreach_established_tcp_flow(flow) {
if (enable)
rc = tcp_flow_repair_on(c, &flow->tcp);
else
@@ -1005,7 +1007,7 @@ int flow_migrate_source(struct ctx *c, const struct migrate_stage
*stage,
(void)c;
(void)stage;
- foreach_established_tcp_flow(flow, FLOW_MAX)
+ foreach_established_tcp_flow(flow)
count++;
count = htonl(count);
@@ -1024,7 +1026,7 @@ int flow_migrate_source(struct ctx *c, const struct migrate_stage
*stage,
* stream might now be inconsistent, and we might have closed listening
* TCP sockets, so just terminate.
*/
- foreach_established_tcp_flow(flow, FLOW_MAX) {
+ foreach_established_tcp_flow(flow) {
rc = tcp_flow_migrate_source(fd, &flow->tcp);
if (rc) {
err("Can't send data, flow %u: %s", FLOW_IDX(flow),
@@ -1051,7 +1053,7 @@ int flow_migrate_source(struct ctx *c, const struct migrate_stage
*stage,
* failures but not if the stream might be inconsistent (reported here
* as EIO).
*/
- foreach_established_tcp_flow(flow, FLOW_MAX) {
+ foreach_established_tcp_flow(flow) {
rc = tcp_flow_migrate_source_ext(fd, &flow->tcp);
if (rc) {
err("Extended data for flow %u: %s", FLOW_IDX(flow),
--
2.48.1
The migration code introduced a number of 'foreach' macros to traverse the
flow table. These aren't inherently tied to migration, so polish up their
naming, move them to flow_table.h and also use in flow_defer_handler()
which is the other place we need to traverse the whole table.
For now we keep foreach_established_tcp_flow() as is.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
flow.c | 36 ++++++++----------------------------
flow_table.h | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 28 deletions(-)
diff --git a/flow.c b/flow.c
index 602fea79..bb5dcc3c 100644
--- a/flow.c
+++ b/flow.c
@@ -53,28 +53,8 @@ const uint8_t flow_proto[] = {
static_assert(ARRAY_SIZE(flow_proto) == FLOW_NUM_TYPES,
"flow_proto[] doesn't match enum flow_type");
-#define foreach_flow(flow) \
- for ((flow) = flowtab; FLOW_IDX(flow) < FLOW_MAX; (flow)++) \
- if ((flow)->f.state == FLOW_STATE_FREE) \
- (flow) += (flow)->free.n - 1; \
- else
-
-#define foreach_active_flow(flow) \
- foreach_flow((flow)) \
- if ((flow)->f.state != FLOW_STATE_ACTIVE) \
- /* NOLINTNEXTLINE(bugprone-branch-clone) */ \
- continue; \
- else
-
-#define foreach_tcp_flow(flow) \
- foreach_active_flow((flow)) \
- if ((flow)->f.type != FLOW_TCP) \
- /* NOLINTNEXTLINE(bugprone-branch-clone) */ \
- continue; \
- else
-
#define foreach_established_tcp_flow(flow) \
- foreach_tcp_flow((flow)) \
+ flow_foreach_of_type((flow), FLOW_TCP) \
if (!tcp_flow_is_established(&(flow)->tcp)) \
/* NOLINTNEXTLINE(bugprone-branch-clone) */ \
continue; \
@@ -801,7 +781,7 @@ void flow_defer_handler(const struct ctx *c, const struct timespec
*now)
struct flow_free_cluster *free_head = NULL;
unsigned *last_next = &flow_first_free;
bool timer = false;
- unsigned idx;
+ union flow *flow;
if (timespec_diff_ms(now, &flow_timer_run) >= FLOW_TIMER_INTERVAL) {
timer = true;
@@ -810,8 +790,7 @@ void flow_defer_handler(const struct ctx *c, const struct timespec
*now)
ASSERT(!flow_new_entry); /* Incomplete flow at end of cycle */
- for (idx = 0; idx < FLOW_MAX; idx++) {
- union flow *flow = &flowtab[idx];
+ flow_foreach_slot(flow) {
bool closed = false;
switch (flow->f.state) {
@@ -828,12 +807,12 @@ void flow_defer_handler(const struct ctx *c, const struct timespec
*now)
} else {
/* New free cluster, add to chain */
free_head = &flow->free;
- *last_next = idx;
+ *last_next = FLOW_IDX(flow);
last_next = &free_head->next;
}
/* Skip remaining empty entries */
- idx += skip - 1;
+ flow += skip - 1;
continue;
}
@@ -886,14 +865,15 @@ void flow_defer_handler(const struct ctx *c, const struct timespec
*now)
if (free_head) {
/* Add slot to current free cluster */
- ASSERT(idx == FLOW_IDX(free_head) + free_head->n);
+ ASSERT(FLOW_IDX(flow) ==
+ FLOW_IDX(free_head) + free_head->n);
free_head->n++;
flow->free.n = flow->free.next = 0;
} else {
/* Create new free cluster */
free_head = &flow->free;
free_head->n = 1;
- *last_next = idx;
+ *last_next = FLOW_IDX(flow);
last_next = &free_head->next;
}
} else {
diff --git a/flow_table.h b/flow_table.h
index 9a2ff24a..fd2c57b9 100644
--- a/flow_table.h
+++ b/flow_table.h
@@ -50,6 +50,42 @@ extern union flow flowtab[];
#define flow_foreach_sidei(sidei_) \
for ((sidei_) = INISIDE; (sidei_) < SIDES; (sidei_)++)
+
+/**
+ * flow_foreach_slot() - Step through each flow table entry
+ * @flow: Takes values of pointer to each flow table entry
+ *
+ * Includes FREE slots.
+ */
+#define flow_foreach_slot(flow) \
+ for ((flow) = flowtab; FLOW_IDX(flow) < FLOW_MAX; (flow)++)
+
+/**
+ * flow_foreach() - Step through each active flow
+ * @flow: Takes values of pointer to each active flow
+ */
+#define flow_foreach(flow) \
+ flow_foreach_slot((flow)) \
+ if ((flow)->f.state == FLOW_STATE_FREE) \
+ (flow) += (flow)->free.n - 1; \
+ else if ((flow)->f.state != FLOW_STATE_ACTIVE) { \
+ flow_err((flow), "Bad flow state during traversal"); \
+ continue; \
+ } else
+
+/**
+ * flow_foreach_of_type() - Step through each active flow of given type
+ * @flow: Takes values of pointer to each flow
+ * @type_: Type of flow to traverse
+ */
+#define flow_foreach_of_type(flow, type_) \
+ flow_foreach((flow)) \
+ if ((flow)->f.type != (type_)) \
+ /* NOLINTNEXTLINE(bugprone-branch-clone) */ \
+ continue; \
+ else
+
+
/** flow_idx() - Index of flow from common structure
* @f: Common flow fields pointer
*
--
2.48.1
On Wed, 19 Feb 2025 13:28:33 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:The migration code introduced a number of flow traversal macros. This series cleans up their interfaces a bit, and makes them more widely useful. David Gibson (3): flow: Remove unneeded index from foreach_* macros flow: Remove unneeded bound parameter from flow traversal macros flow: Clean up and generalise flow traversal macros flow.c | 72 +++++++++++++++++++--------------------------------- flow_table.h | 36 ++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 46 deletions(-)Applied. -- Stefano