DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments
@ 2015-12-19 22:39 David Verbeiren
  2016-04-01 14:12 ` Thomas Monjalon
  2016-04-07 11:00 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
  0 siblings, 2 replies; 6+ messages in thread
From: David Verbeiren @ 2015-12-19 22:39 UTC (permalink / raw)
  To: dev, 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments
  2015-12-19 22:39 [dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments David Verbeiren
@ 2016-04-01 14:12 ` Thomas Monjalon
  2016-04-01 14:46   ` Burakov, Anatoly
  2016-04-07 11:00 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
  1 sibling, 1 reply; 6+ messages in thread
From: Thomas Monjalon @ 2016-04-01 14:12 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev, david.verbeiren

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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments
  2016-04-01 14:12 ` Thomas Monjalon
@ 2016-04-01 14:46   ` Burakov, Anatoly
  2016-04-01 15:59     ` Burakov, Anatoly
  0 siblings, 1 reply; 6+ messages in thread
From: Burakov, Anatoly @ 2016-04-01 14:46 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, david.verbeiren

> 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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments
  2016-04-01 14:46   ` Burakov, Anatoly
@ 2016-04-01 15:59     ` Burakov, Anatoly
  0 siblings, 0 replies; 6+ messages in thread
From: Burakov, Anatoly @ 2016-04-01 15:59 UTC (permalink / raw)
  To: Burakov, Anatoly, Thomas Monjalon; +Cc: dev, david.verbeiren

> > 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [dpdk-dev] [PATCH v2] ivshmem: avoid infinite loop when concatenating adjacent segments
  2015-12-19 22:39 [dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments David Verbeiren
  2016-04-01 14:12 ` Thomas Monjalon
@ 2016-04-07 11:00 ` Anatoly Burakov
  2016-04-07 17:18   ` Thomas Monjalon
  1 sibling, 1 reply; 6+ messages in thread
From: Anatoly Burakov @ 2016-04-07 11:00 UTC (permalink / raw)
  To: dev; +Cc: david.verbeiren, 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>
---
 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v2] ivshmem: avoid infinite loop when concatenating adjacent segments
  2016-04-07 11:00 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
@ 2016-04-07 17:18   ` Thomas Monjalon
  0 siblings, 0 replies; 6+ messages in thread
From: Thomas Monjalon @ 2016-04-07 17:18 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev, 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. 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-04-07 17:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-19 22:39 [dpdk-dev] [PATCH] ivshmem: avoid infinite loop when concatenating adjacent segments David Verbeiren
2016-04-01 14:12 ` Thomas Monjalon
2016-04-01 14:46   ` Burakov, Anatoly
2016-04-01 15:59     ` Burakov, Anatoly
2016-04-07 11:00 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
2016-04-07 17:18   ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).