[dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments

[dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments

[David Verbeiren]

This patch aligns the logic used to check for the presence of
adjacent segments in has_adjacent_segments() with the logic used
in cleanup_segments() when actually deciding to concatenate or
not a pair of segments.

This fixes an infinite loop that happened when segments where
adjacent in their physical or virtual addresses but not in their
ioremap addresses: has_adjacent_segments() reported the presence
of adjacent segments while cleanup_segments() was not considering
them for concatenation, resulting in an infinite loop since the
result of has_adjacent_segments() is used in the decision to
continue looping in cleanup_segments().

Signed-off-by: David Verbeiren <david.verbeiren@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_ivshmem.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_ivshmem.c b/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
index 589019b..086cafb 100644
--- a/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
+++ b/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
@@ -254,17 +254,18 @@ adjacent(const struct rte_memzone * mz1, const struct rte_memzone * mz2)
 static int
 has_adjacent_segments(struct ivshmem_segment * ms, int len)
 {
-	int i, j, a;
+	int i, j;
 
 	for (i = 0; i < len; i++)
 		for (j = i + 1; j < len; j++) {
-			a = adjacent(&ms[i].entry.mz, &ms[j].entry.mz);
-
-			/* check if segments are adjacent virtually and/or physically but
-			 * not ioremap (since that would indicate that they are from
-			 * different PCI devices and thus don't need to be concatenated.
+			
+			/* check if segments are adjacent virtually and physically.
+			 * They must also be adjacent in ioremap since disjoint
+			 * ioremap segments would be from different PCI devices and
+			 * thus don't need to be concatenated. This is the logic 
+			 * used in cleanup_segments() and must be in sync.
 			 */
-			if ((a & (VIRT|PHYS)) > 0 && (a & IOREMAP) == 0)
+			if (adjacent(&ms[i].entry.mz, &ms[j].entry.mz) == FULL)
 				return 1;
 		}
 	return 0;
-- 
1.9.1

Re: [dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments

[Thomas Monjalon]

Please Anatoly,
What do you think of this patch?

2015-12-19 23:39, David Verbeiren:
> This patch aligns the logic used to check for the presence of
> adjacent segments in has_adjacent_segments() with the logic used
> in cleanup_segments() when actually deciding to concatenate or
> not a pair of segments.
> 
> This fixes an infinite loop that happened when segments where
> adjacent in their physical or virtual addresses but not in their
> ioremap addresses: has_adjacent_segments() reported the presence
> of adjacent segments while cleanup_segments() was not considering
> them for concatenation, resulting in an infinite loop since the
> result of has_adjacent_segments() is used in the decision to
> continue looping in cleanup_segments().
> 
> Signed-off-by: David Verbeiren <david.verbeiren@intel.com>

Re: [dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments

[Burakov, Anatoly]

> Please Anatoly,
> What do you think of this patch?
> 
> 2015-12-19 23:39, David Verbeiren:
> > This patch aligns the logic used to check for the presence of adjacent
> > segments in has_adjacent_segments() with the logic used in
> > cleanup_segments() when actually deciding to concatenate or not a pair
> > of segments.
> >
> > This fixes an infinite loop that happened when segments where adjacent
> > in their physical or virtual addresses but not in their ioremap
> > addresses: has_adjacent_segments() reported the presence of adjacent
> > segments while cleanup_segments() was not considering them for
> > concatenation, resulting in an infinite loop since the result of
> > has_adjacent_segments() is used in the decision to continue looping in
> > cleanup_segments().
> >
> > Signed-off-by: David Verbeiren <david.verbeiren@intel.com>

Yes, looking back on this, it made no sense. Or rather it did make some twisted sense, but led to a bug. So,

Acked-by: Anatoly  Burakov <anatoly.burakov@intel.com>

Re: [dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments

[Burakov, Anatoly]

> > Please Anatoly,
> > What do you think of this patch?
> >
> > 2015-12-19 23:39, David Verbeiren:
> > > This patch aligns the logic used to check for the presence of
> > > adjacent segments in has_adjacent_segments() with the logic used in
> > > cleanup_segments() when actually deciding to concatenate or not a
> > > pair of segments.
> > >
> > > This fixes an infinite loop that happened when segments where
> > > adjacent in their physical or virtual addresses but not in their
> > > ioremap
> > > addresses: has_adjacent_segments() reported the presence of adjacent
> > > segments while cleanup_segments() was not considering them for
> > > concatenation, resulting in an infinite loop since the result of
> > > has_adjacent_segments() is used in the decision to continue looping
> > > in cleanup_segments().
> > >
> > > Signed-off-by: David Verbeiren <david.verbeiren@intel.com>
> 
> Yes, looking back on this, it made no sense. Or rather it did make some
> twisted sense, but led to a bug. So,
> 
> Acked-by: Anatoly  Burakov <anatoly.burakov@intel.com>
> 
> 

Actually, scratch that. I think additional changes are needed. This patch is OK, but it is incomplete and introduces a different bug.

If the segments aren't aren't fully adjacent, that would still cause "overlaps" to be non-zero since it expects everything to be >= start and < end. If start1 == start2, that segment is both adjacent and overlapping. So in order to avoid throwing errors when not fully adjacent, but not overlapping, segments are present, we need to also fix the overlapping() function to report only overlaps and not adjacency.

Thanks,
Anatoly

[dpdk-dev] [PATCH v2] ivshmem: avoid infinite loop when concatenating adjacent segments

[Anatoly Burakov]

This patch aligns the logic used to check for the presence of
adjacent segments in has_adjacent_segments() with the logic used
in cleanup_segments() when actually deciding to concatenate or
not a pair of segments. Additionally, adjacent segments are
no longer considered overlapping to avoid generating errors for
segments that can happily coexist together.

This fixes an infinite loop that happened when segments where
adjacent in their physical or virtual addresses but not in their
ioremap addresses: has_adjacent_segments() reported the presence
of adjacent segments while cleanup_segments() was not considering
them for concatenation, resulting in an infinite loop since the
result of has_adjacent_segments() is used in the decision to
continue looping in cleanup_segments().

Signed-off-by: David Verbeiren <david.verbeiren@intel.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_ivshmem.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_ivshmem.c b/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
index 28ddf09..07aec69 100644
--- a/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
+++ b/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
@@ -184,21 +184,21 @@ overlap(const struct rte_memzone * mz1, const struct rte_memzone * mz2)
 	i_end2 = mz2->ioremap_addr + mz2->len;
 
 	/* check for overlap in virtual addresses */
-	if (start1 >= start2 && start1 < end2)
+	if (start1 > start2 && start1 < end2)
 		result |= VIRT;
 	if (start2 >= start1 && start2 < end1)
 		result |= VIRT;
 
 	/* check for overlap in physical addresses */
-	if (p_start1 >= p_start2 && p_start1 < p_end2)
+	if (p_start1 > p_start2 && p_start1 < p_end2)
 		result |= PHYS;
-	if (p_start2 >= p_start1 && p_start2 < p_end1)
+	if (p_start2 > p_start1 && p_start2 < p_end1)
 		result |= PHYS;
 
 	/* check for overlap in ioremap addresses */
-	if (i_start1 >= i_start2 && i_start1 < i_end2)
+	if (i_start1 > i_start2 && i_start1 < i_end2)
 		result |= IOREMAP;
-	if (i_start2 >= i_start1 && i_start2 < i_end1)
+	if (i_start2 > i_start1 && i_start2 < i_end1)
 		result |= IOREMAP;
 
 	return result;
@@ -254,17 +254,14 @@ adjacent(const struct rte_memzone * mz1, const struct rte_memzone * mz2)
 static int
 has_adjacent_segments(struct ivshmem_segment * ms, int len)
 {
-	int i, j, a;
+	int i, j;
 
 	for (i = 0; i < len; i++)
 		for (j = i + 1; j < len; j++) {
-			a = adjacent(&ms[i].entry.mz, &ms[j].entry.mz);
-
-			/* check if segments are adjacent virtually and/or physically but
-			 * not ioremap (since that would indicate that they are from
-			 * different PCI devices and thus don't need to be concatenated.
+			/* we're only interested in fully adjacent segments; partially
+			 * adjacent segments can coexist.
 			 */
-			if ((a & (VIRT|PHYS)) > 0 && (a & IOREMAP) == 0)
+			if (adjacent(&ms[i].entry.mz, &ms[j].entry.mz) == FULL)
 				return 1;
 		}
 	return 0;
-- 
2.5.0

Re: [dpdk-dev] [PATCH v2] ivshmem: avoid infinite loop when concatenating adjacent segments

[Thomas Monjalon]

> This patch aligns the logic used to check for the presence of
> adjacent segments in has_adjacent_segments() with the logic used
> in cleanup_segments() when actually deciding to concatenate or
> not a pair of segments. Additionally, adjacent segments are
> no longer considered overlapping to avoid generating errors for
> segments that can happily coexist together.
> 
> This fixes an infinite loop that happened when segments where
> adjacent in their physical or virtual addresses but not in their
> ioremap addresses: has_adjacent_segments() reported the presence
> of adjacent segments while cleanup_segments() was not considering
> them for concatenation, resulting in an infinite loop since the
> result of has_adjacent_segments() is used in the decision to
> continue looping in cleanup_segments().
> 
> Signed-off-by: David Verbeiren <david.verbeiren@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks