diff mbox

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

Message ID 1450564772-20000-1-git-send-email-david.verbeiren@intel.com (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Verbeiren, David Dec. 19, 2015, 10:39 p.m. UTC 
  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(-)

Comments

Thomas Monjalon April 1, 2016, 2:12 p.m. UTC | #1 
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>
Burakov, Anatoly April 1, 2016, 2:46 p.m. UTC | #2 
> 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>
Burakov, Anatoly April 1, 2016, 3:59 p.m. UTC | #3 
> > 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
diff mbox

Patch

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;