[dpdk-dev] [PATCH] eal:ppc: fix incorrect ifdef for ppc_64

[dpdk-dev] [PATCH] eal:ppc: fix incorrect ifdef for ppc_64

[David Christensen]

An ifdef present in eal_memory.c references "RTE_ARCH_PPC64" when
it should actually use "RTE_ARCH_PPC_64".  Simple testing revealed
that both the PPC_64 and non-PPC_64 versions of the code involved
work, but the PPC_64 version of the code is retained to be
consistent with other instances in the same file where mmapped
memory is accessed in reverse order on Power platforms.

Fixes: 66cc45e ("mem: replace memseg with memseg lists")
Cc: Anatoly Burakov <anatoly.burakov@intel.com>

Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
---
 lib/librte_eal/linux/eal/eal_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linux/eal/eal_memory.c b/lib/librte_eal/linux/eal/eal_memory.c
index 1c089a1ef..1bbdd8a29 100644
--- a/lib/librte_eal/linux/eal/eal_memory.c
+++ b/lib/librte_eal/linux/eal/eal_memory.c
@@ -688,7 +688,7 @@ remap_segment(struct hugepage_file *hugepages, int seg_start, int seg_end)
 		return -1;
 	}
 
-#ifdef RTE_ARCH_PPC64
+#ifdef RTE_ARCH_PPC_64
 	/* for PPC64 we go through the list backwards */
 	for (cur_page = seg_end - 1; cur_page >= seg_start;
 			cur_page--, ms_idx++) {
-- 
2.18.1

Re: [dpdk-dev] [PATCH] eal:ppc: fix incorrect ifdef for ppc_64

[Ferruh Yigit]

On 9/25/2019 10:42 PM, David Christensen wrote:
> An ifdef present in eal_memory.c references "RTE_ARCH_PPC64" when
> it should actually use "RTE_ARCH_PPC_64".  Simple testing revealed
> that both the PPC_64 and non-PPC_64 versions of the code involved
> work, but the PPC_64 version of the code is retained to be
> consistent with other instances in the same file where mmapped
> memory is accessed in reverse order on Power platforms.
> 
> Fixes: 66cc45e ("mem: replace memseg with memseg lists")
> Cc: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> ---
>  lib/librte_eal/linux/eal/eal_memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/linux/eal/eal_memory.c b/lib/librte_eal/linux/eal/eal_memory.c
> index 1c089a1ef..1bbdd8a29 100644
> --- a/lib/librte_eal/linux/eal/eal_memory.c
> +++ b/lib/librte_eal/linux/eal/eal_memory.c
> @@ -688,7 +688,7 @@ remap_segment(struct hugepage_file *hugepages, int seg_start, int seg_end)
>  		return -1;
>  	}
>  
> -#ifdef RTE_ARCH_PPC64
> +#ifdef RTE_ARCH_PPC_64
>  	/* for PPC64 we go through the list backwards */
>  	for (cur_page = seg_end - 1; cur_page >= seg_start;
>  			cur_page--, ms_idx++) {
> 

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH] eal:ppc: fix incorrect ifdef for ppc_64

[David Marchand]

On Thu, Sep 26, 2019 at 9:44 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 9/25/2019 10:42 PM, David Christensen wrote:
> > An ifdef present in eal_memory.c references "RTE_ARCH_PPC64" when
> > it should actually use "RTE_ARCH_PPC_64".  Simple testing revealed
> > that both the PPC_64 and non-PPC_64 versions of the code involved
> > work, but the PPC_64 version of the code is retained to be
> > consistent with other instances in the same file where mmapped
> > memory is accessed in reverse order on Power platforms.
> >
> > Fixes: 66cc45e ("mem: replace memseg with memseg lists")
Cc: stable@dpdk.org

> >
> > Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied, thanks.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal:ppc: fix incorrect ifdef for ppc_64

[David Marchand]

On Wed, Sep 25, 2019 at 11:42 PM David Christensen
<drc@linux.vnet.ibm.com> wrote:
>
> An ifdef present in eal_memory.c references "RTE_ARCH_PPC64" when
> it should actually use "RTE_ARCH_PPC_64".  Simple testing revealed
> that both the PPC_64 and non-PPC_64 versions of the code involved
> work, but the PPC_64 version of the code is retained to be
> consistent with other instances in the same file where mmapped
> memory is accessed in reverse order on Power platforms.

The change itself is not that scary, but just reading this commitlog I
fail to see the impact for an application.
Can you share some light?


>
> Fixes: 66cc45e ("mem: replace memseg with memseg lists")

Fixes sha1 should be on 12 digits.
Should we copy stable ?

> Cc: Anatoly Burakov <anatoly.burakov@intel.com>
>
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> ---
>  lib/librte_eal/linux/eal/eal_memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/linux/eal/eal_memory.c b/lib/librte_eal/linux/eal/eal_memory.c
> index 1c089a1ef..1bbdd8a29 100644
> --- a/lib/librte_eal/linux/eal/eal_memory.c
> +++ b/lib/librte_eal/linux/eal/eal_memory.c
> @@ -688,7 +688,7 @@ remap_segment(struct hugepage_file *hugepages, int seg_start, int seg_end)
>                 return -1;
>         }
>
> -#ifdef RTE_ARCH_PPC64
> +#ifdef RTE_ARCH_PPC_64
>         /* for PPC64 we go through the list backwards */
>         for (cur_page = seg_end - 1; cur_page >= seg_start;
>                         cur_page--, ms_idx++) {
> --
> 2.18.1
>

Thanks.

-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal:ppc: fix incorrect ifdef for ppc_64

[David Christensen]

>> An ifdef present in eal_memory.c references "RTE_ARCH_PPC64" when
>> it should actually use "RTE_ARCH_PPC_64".  Simple testing revealed
>> that both the PPC_64 and non-PPC_64 versions of the code involved
>> work, but the PPC_64 version of the code is retained to be
>> consistent with other instances in the same file where mmapped
>> memory is accessed in reverse order on Power platforms.
> 
> The change itself is not that scary, but just reading this commitlog I
> fail to see the impact for an application.
> Can you share some light?
> 

As far as I can tell there is no impact on any applications.  The old 
code, which walked through the list in a forward direction, worked 
perfectly well with testpmd and DPDK pktgen applications on Power systems.

With the ifdef fixed, the core walks the list in the reverse direction 
as intended, the code still worked (i.e. no errors or problems were 
observed in the same test applications).

I'm not completely familiar with why memseg lists must be traversed in 
the reverse direction for Power systems.  It might be something specific 
to Power 8 systems which I'm not actually supporting on DPDK, only the 
Power 9 systems that I use for for development and testing.

Dave

Re: [dpdk-dev] [PATCH] eal:ppc: fix incorrect ifdef for ppc_64

[Burakov, Anatoly]

On 16-Oct-19 9:45 PM, David Christensen wrote:
>>> An ifdef present in eal_memory.c references "RTE_ARCH_PPC64" when
>>> it should actually use "RTE_ARCH_PPC_64".  Simple testing revealed
>>> that both the PPC_64 and non-PPC_64 versions of the code involved
>>> work, but the PPC_64 version of the code is retained to be
>>> consistent with other instances in the same file where mmapped
>>> memory is accessed in reverse order on Power platforms.
>>
>> The change itself is not that scary, but just reading this commitlog I
>> fail to see the impact for an application.
>> Can you share some light?
>>
> 
> As far as I can tell there is no impact on any applications.  The old 
> code, which walked through the list in a forward direction, worked 
> perfectly well with testpmd and DPDK pktgen applications on Power systems.
> 
> With the ifdef fixed, the core walks the list in the reverse direction 
> as intended, the code still worked (i.e. no errors or problems were 
> observed in the same test applications).
> 
> I'm not completely familiar with why memseg lists must be traversed in 
> the reverse direction for Power systems.  It might be something specific 
> to Power 8 systems which I'm not actually supporting on DPDK, only the 
> Power 9 systems that I use for for development and testing.
> 
> Dave
> 

If the code makes no difference anyway, should we just take it out so?

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] eal:ppc: fix incorrect ifdef for ppc_64

[David Marchand]

On Thu, Oct 17, 2019 at 6:18 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 16-Oct-19 9:45 PM, David Christensen wrote:
> >>> An ifdef present in eal_memory.c references "RTE_ARCH_PPC64" when
> >>> it should actually use "RTE_ARCH_PPC_64".  Simple testing revealed
> >>> that both the PPC_64 and non-PPC_64 versions of the code involved
> >>> work, but the PPC_64 version of the code is retained to be
> >>> consistent with other instances in the same file where mmapped
> >>> memory is accessed in reverse order on Power platforms.
> >>
> >> The change itself is not that scary, but just reading this commitlog I
> >> fail to see the impact for an application.
> >> Can you share some light?
> >>
> >
> > As far as I can tell there is no impact on any applications.  The old
> > code, which walked through the list in a forward direction, worked
> > perfectly well with testpmd and DPDK pktgen applications on Power systems.
> >
> > With the ifdef fixed, the core walks the list in the reverse direction
> > as intended, the code still worked (i.e. no errors or problems were
> > observed in the same test applications).
> >
> > I'm not completely familiar with why memseg lists must be traversed in
> > the reverse direction for Power systems.  It might be something specific
> > to Power 8 systems which I'm not actually supporting on DPDK, only the
> > Power 9 systems that I use for for development and testing.
> >
> > Dave
> >
>
> If the code makes no difference anyway, should we just take it out so?

+1 :-)


-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal:ppc: fix incorrect ifdef for ppc_64

[David Christensen]

>>>> The change itself is not that scary, but just reading this commitlog I
>>>> fail to see the impact for an application.
>>>> Can you share some light?
>>>>
>>>
>>> As far as I can tell there is no impact on any applications.  The old
>>> code, which walked through the list in a forward direction, worked
>>> perfectly well with testpmd and DPDK pktgen applications on Power systems.
>>>
>>> With the ifdef fixed, the core walks the list in the reverse direction
>>> as intended, the code still worked (i.e. no errors or problems were
>>> observed in the same test applications).
>>>
>>> I'm not completely familiar with why memseg lists must be traversed in
>>> the reverse direction for Power systems.  It might be something specific
>>> to Power 8 systems which I'm not actually supporting on DPDK, only the
>>> Power 9 systems that I use for for development and testing.
>>>
>> If the code makes no difference anyway, should we just take it out so?
> 
> +1 :-)

I think there's a need for a larger review of Power8 vs. Power9 support. 
  You currently need to specify Power8 as the DPDK build target (e.g. 
ppc_64-power8-linux-gcc) but all of our internal development and testing 
efforts are targeting Power9 systems.  My preference would be to drop 
Power8 support all together but I'm reluctant to make such a potentially 
large change so close to an LTS release target, and not without 
soliciting some community comment on the idea.  As a result, I'd prefer 
to keep the change "as is" for this release.

Dave

Re: [dpdk-dev] [PATCH] eal:ppc: fix incorrect ifdef for ppc_64

[David Marchand]

On Thu, Oct 17, 2019 at 6:56 PM David Christensen
<drc@linux.vnet.ibm.com> wrote:
>
> >>>> The change itself is not that scary, but just reading this commitlog I
> >>>> fail to see the impact for an application.
> >>>> Can you share some light?
> >>>>
> >>>
> >>> As far as I can tell there is no impact on any applications.  The old
> >>> code, which walked through the list in a forward direction, worked
> >>> perfectly well with testpmd and DPDK pktgen applications on Power systems.
> >>>
> >>> With the ifdef fixed, the core walks the list in the reverse direction
> >>> as intended, the code still worked (i.e. no errors or problems were
> >>> observed in the same test applications).
> >>>
> >>> I'm not completely familiar with why memseg lists must be traversed in
> >>> the reverse direction for Power systems.  It might be something specific
> >>> to Power 8 systems which I'm not actually supporting on DPDK, only the
> >>> Power 9 systems that I use for for development and testing.
> >>>
> >> If the code makes no difference anyway, should we just take it out so?
> >
> > +1 :-)
>
> I think there's a need for a larger review of Power8 vs. Power9 support.
>   You currently need to specify Power8 as the DPDK build target (e.g.
> ppc_64-power8-linux-gcc) but all of our internal development and testing
> efforts are targeting Power9 systems.  My preference would be to drop
> Power8 support all together but I'm reluctant to make such a potentially
> large change so close to an LTS release target, and not without
> soliciting some community comment on the idea.  As a result, I'd prefer
> to keep the change "as is" for this release.

Ok, I will take it as is, but please do investigate this.
The lesser special cases like these, the better.


-- 
David Marchand