[dpdk-dev,v2,11/12] mempool: honor iova mode in virt2phy
Checks
Commit Message
Check iova mode and accordingly return phy addr.
Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
lib/librte_mempool/rte_mempool.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
Comments
On Mon, 10 Jul 2017 11:42:34 +0000, Santosh Shukla <santosh.shukla@caviumnetworks.com> wrote:
> Check iova mode and accordingly return phy addr.
>
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
> lib/librte_mempool/rte_mempool.h | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> index 76b5b3b15..fafa77e3b 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -78,6 +78,7 @@
> #include <rte_ring.h>
> #include <rte_memcpy.h>
> #include <rte_common.h>
> +#include <rte_bus.h>
>
> #ifdef __cplusplus
> extern "C" {
> @@ -1397,9 +1398,14 @@ rte_mempool_empty(const struct rte_mempool *mp)
> static inline phys_addr_t
> rte_mempool_virt2phy(__rte_unused const struct rte_mempool *mp, const void *elt)
> {
> - const struct rte_mempool_objhdr *hdr;
> - hdr = (const struct rte_mempool_objhdr *)RTE_PTR_SUB(elt,
> + struct rte_mempool_objhdr *hdr;
> +
> + hdr = (struct rte_mempool_objhdr *)RTE_PTR_SUB(elt,
> sizeof(*hdr));
> +
> + if (rte_eal_iova_mode() == RTE_IOVA_VA)
> + hdr->physaddr = (uintptr_t)elt;
> +
> return hdr->physaddr;
> }
>
This overrides the physaddr field in the object hdr, this is
surely not what you want (note that hdr was const).
This change could at least take place in mempool_add_elem().
There is even maybe no need to change rte_mempool at all: if
rte_memzone_reserve() already returns the proper address in
memzone->phys_addr, it should be transparent.
I didn't check the patchset in detail, but in my understanding,
what we call physaddr in dpdk is actually a bus address. Shouldn't
we start to rename some of these fields and functions to avoid
confusion?
Thanks,
Olivier
Hi Olivier,
On Monday 10 July 2017 05:57 PM, Olivier Matz wrote:
> On Mon, 10 Jul 2017 11:42:34 +0000, Santosh Shukla <santosh.shukla@caviumnetworks.com> wrote:
>> Check iova mode and accordingly return phy addr.
>>
>> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> ---
>> lib/librte_mempool/rte_mempool.h | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
>> index 76b5b3b15..fafa77e3b 100644
>> --- a/lib/librte_mempool/rte_mempool.h
>> +++ b/lib/librte_mempool/rte_mempool.h
>> @@ -78,6 +78,7 @@
>> #include <rte_ring.h>
>> #include <rte_memcpy.h>
>> #include <rte_common.h>
>> +#include <rte_bus.h>
>>
>> #ifdef __cplusplus
>> extern "C" {
>> @@ -1397,9 +1398,14 @@ rte_mempool_empty(const struct rte_mempool *mp)
>> static inline phys_addr_t
>> rte_mempool_virt2phy(__rte_unused const struct rte_mempool *mp, const void *elt)
>> {
>> - const struct rte_mempool_objhdr *hdr;
>> - hdr = (const struct rte_mempool_objhdr *)RTE_PTR_SUB(elt,
>> + struct rte_mempool_objhdr *hdr;
>> +
>> + hdr = (struct rte_mempool_objhdr *)RTE_PTR_SUB(elt,
>> sizeof(*hdr));
>> +
>> + if (rte_eal_iova_mode() == RTE_IOVA_VA)
>> + hdr->physaddr = (uintptr_t)elt;
>> +
>> return hdr->physaddr;
>> }
>>
> This overrides the physaddr field in the object hdr, this is
> surely not what you want (note that hdr was const).
>
> This change could at least take place in mempool_add_elem().
> There is even maybe no need to change rte_mempool at all: if
> rte_memzone_reserve() already returns the proper address in
> memzone->phys_addr, it should be transparent.
Indeed. The change at rte_malloc_virt2phy() make sure
that mz->phys_addr has va_addr in case iova=va mapping mode.
So its transparent for both modes.
virt2phy translation for mempool like derivative
class api not required. Provided that rte_mem/malloc_virt2phy()
apis are iova mode aware.
> I didn't check the patchset in detail, but in my understanding,
> what we call physaddr in dpdk is actually a bus address. Shouldn't
> we start to rename some of these fields and functions to avoid
> confusion?
Agree.
While working on iova mode thing and reading these vir2phy api -
confused me more. Actually it should be iova2va, va2iova or pa2iova,iova2pa..
where iova address is nothing but bus address Or we should refer to linux
semantics.
We thought of addressing semantics after this series, Not a priority in IMO.
>
> Thanks,
> Olivier
10/07/2017 15:30, santosh:
> Hi Olivier,
>
> On Monday 10 July 2017 05:57 PM, Olivier Matz wrote:
> > I didn't check the patchset in detail, but in my understanding,
> > what we call physaddr in dpdk is actually a bus address. Shouldn't
> > we start to rename some of these fields and functions to avoid
> > confusion?
>
> Agree.
> While working on iova mode thing and reading these vir2phy api -
> confused me more. Actually it should be iova2va, va2iova or pa2iova,iova2pa..
> where iova address is nothing but bus address Or we should refer to linux
> semantics.
>
> We thought of addressing semantics after this series, Not a priority in IMO.
I think it is a priority to start with semantics.
The work is too hard with wrong semantic otherwise.
On Monday 10 July 2017 07:21 PM, Thomas Monjalon wrote:
> 10/07/2017 15:30, santosh:
>> Hi Olivier,
>>
>> On Monday 10 July 2017 05:57 PM, Olivier Matz wrote:
>>> I didn't check the patchset in detail, but in my understanding,
>>> what we call physaddr in dpdk is actually a bus address. Shouldn't
>>> we start to rename some of these fields and functions to avoid
>>> confusion?
>> Agree.
>> While working on iova mode thing and reading these vir2phy api -
>> confused me more. Actually it should be iova2va, va2iova or pa2iova,iova2pa..
>> where iova address is nothing but bus address Or we should refer to linux
>> semantics.
>>
>> We thought of addressing semantics after this series, Not a priority in IMO.
> I think it is a priority to start with semantics.
> The work is too hard with wrong semantic otherwise.
Sorry, I don;t agree with you. Semantic shouldn't lower the iova priority.
iova framework is blocking SoC's. w/o iova framework : One has to live with
hackish solution for their SoC.
Semantic change in any-case could be pipelined. It shouldn't be like
Semantics change gets priority and therefore it blocks other SoCs.
10/07/2017 15:56, santosh:
> On Monday 10 July 2017 07:21 PM, Thomas Monjalon wrote:
>
> > 10/07/2017 15:30, santosh:
> >> Hi Olivier,
> >>
> >> On Monday 10 July 2017 05:57 PM, Olivier Matz wrote:
> >>> I didn't check the patchset in detail, but in my understanding,
> >>> what we call physaddr in dpdk is actually a bus address. Shouldn't
> >>> we start to rename some of these fields and functions to avoid
> >>> confusion?
> >> Agree.
> >> While working on iova mode thing and reading these vir2phy api -
> >> confused me more. Actually it should be iova2va, va2iova or pa2iova,iova2pa..
> >> where iova address is nothing but bus address Or we should refer to linux
> >> semantics.
> >>
> >> We thought of addressing semantics after this series, Not a priority in IMO.
> > I think it is a priority to start with semantics.
> > The work is too hard with wrong semantic otherwise.
>
> Sorry, I don;t agree with you. Semantic shouldn't lower the iova priority.
> iova framework is blocking SoC's. w/o iova framework : One has to live with
> hackish solution for their SoC.
>
> Semantic change in any-case could be pipelined. It shouldn't be like
> Semantics change gets priority and therefore it blocks other SoCs.
I am not saying it is blocking.
I just say that you have not started your work by the beginning,
and now it make reviews difficult (from what I understand).
You must make all the efforts to make your patches easier to
understand and accept.
On Monday 10 July 2017 07:39 PM, Thomas Monjalon wrote:
> 10/07/2017 15:56, santosh:
>> On Monday 10 July 2017 07:21 PM, Thomas Monjalon wrote:
>>
>>> 10/07/2017 15:30, santosh:
>>>> Hi Olivier,
>>>>
>>>> On Monday 10 July 2017 05:57 PM, Olivier Matz wrote:
>>>>> I didn't check the patchset in detail, but in my understanding,
>>>>> what we call physaddr in dpdk is actually a bus address. Shouldn't
>>>>> we start to rename some of these fields and functions to avoid
>>>>> confusion?
>>>> Agree.
>>>> While working on iova mode thing and reading these vir2phy api -
>>>> confused me more. Actually it should be iova2va, va2iova or pa2iova,iova2pa..
>>>> where iova address is nothing but bus address Or we should refer to linux
>>>> semantics.
>>>>
>>>> We thought of addressing semantics after this series, Not a priority in IMO.
>>> I think it is a priority to start with semantics.
>>> The work is too hard with wrong semantic otherwise.
>> Sorry, I don;t agree with you. Semantic shouldn't lower the iova priority.
>> iova framework is blocking SoC's. w/o iova framework : One has to live with
>> hackish solution for their SoC.
>>
>> Semantic change in any-case could be pipelined. It shouldn't be like
>> Semantics change gets priority and therefore it blocks other SoCs.
> I am not saying it is blocking.
> I just say that you have not started your work by the beginning,
> and now it make reviews difficult (from what I understand).
> You must make all the efforts to make your patches easier to
> understand and accept.
It's just about changing name for virt2phy api's.. But changing those function
names require deprecation notice, Once iova patchset is merged then I'll
take up responsibility for sending deprecation notice and change those api
name in the next release.
10/07/2017 16:22, santosh:
> On Monday 10 July 2017 07:39 PM, Thomas Monjalon wrote:
>
> > 10/07/2017 15:56, santosh:
> >> On Monday 10 July 2017 07:21 PM, Thomas Monjalon wrote:
> >>
> >>> 10/07/2017 15:30, santosh:
> >>>> Hi Olivier,
> >>>>
> >>>> On Monday 10 July 2017 05:57 PM, Olivier Matz wrote:
> >>>>> I didn't check the patchset in detail, but in my understanding,
> >>>>> what we call physaddr in dpdk is actually a bus address. Shouldn't
> >>>>> we start to rename some of these fields and functions to avoid
> >>>>> confusion?
> >>>> Agree.
> >>>> While working on iova mode thing and reading these vir2phy api -
> >>>> confused me more. Actually it should be iova2va, va2iova or pa2iova,iova2pa..
> >>>> where iova address is nothing but bus address Or we should refer to linux
> >>>> semantics.
> >>>>
> >>>> We thought of addressing semantics after this series, Not a priority in IMO.
> >>> I think it is a priority to start with semantics.
> >>> The work is too hard with wrong semantic otherwise.
> >> Sorry, I don;t agree with you. Semantic shouldn't lower the iova priority.
> >> iova framework is blocking SoC's. w/o iova framework : One has to live with
> >> hackish solution for their SoC.
> >>
> >> Semantic change in any-case could be pipelined. It shouldn't be like
> >> Semantics change gets priority and therefore it blocks other SoCs.
> > I am not saying it is blocking.
> > I just say that you have not started your work by the beginning,
> > and now it make reviews difficult (from what I understand).
> > You must make all the efforts to make your patches easier to
> > understand and accept.
>
> It's just about changing name for virt2phy api's.. But changing those function
> names require deprecation notice, Once iova patchset is merged then I'll
> take up responsibility for sending deprecation notice and change those api
> name in the next release.
This series is not going to be integrated in 17.08.
Anyway, you should probably send the deprecation notice now,
in order to change the semantic in 17.11.
Olivier was also talking about physaddr wording in EAL code.
Hi Thomas,
On Monday 10 July 2017 08:07 PM, Thomas Monjalon wrote:
> 10/07/2017 16:22, santosh:
>> On Monday 10 July 2017 07:39 PM, Thomas Monjalon wrote:
>>
>>> 10/07/2017 15:56, santosh:
>>>> On Monday 10 July 2017 07:21 PM, Thomas Monjalon wrote:
>>>>
>>>>> 10/07/2017 15:30, santosh:
>>>>>> Hi Olivier,
>>>>>>
>>>>>> On Monday 10 July 2017 05:57 PM, Olivier Matz wrote:
>>>>>>> I didn't check the patchset in detail, but in my understanding,
>>>>>>> what we call physaddr in dpdk is actually a bus address. Shouldn't
>>>>>>> we start to rename some of these fields and functions to avoid
>>>>>>> confusion?
>>>>>> Agree.
>>>>>> While working on iova mode thing and reading these vir2phy api -
>>>>>> confused me more. Actually it should be iova2va, va2iova or pa2iova,iova2pa..
>>>>>> where iova address is nothing but bus address Or we should refer to linux
>>>>>> semantics.
>>>>>>
>>>>>> We thought of addressing semantics after this series, Not a priority in IMO.
>>>>> I think it is a priority to start with semantics.
>>>>> The work is too hard with wrong semantic otherwise.
>>>> Sorry, I don;t agree with you. Semantic shouldn't lower the iova priority.
>>>> iova framework is blocking SoC's. w/o iova framework : One has to live with
>>>> hackish solution for their SoC.
>>>>
>>>> Semantic change in any-case could be pipelined. It shouldn't be like
>>>> Semantics change gets priority and therefore it blocks other SoCs.
>>> I am not saying it is blocking.
>>> I just say that you have not started your work by the beginning,
>>> and now it make reviews difficult (from what I understand).
>>> You must make all the efforts to make your patches easier to
>>> understand and accept.
>> It's just about changing name for virt2phy api's.. But changing those function
>> names require deprecation notice, Once iova patchset is merged then I'll
>> take up responsibility for sending deprecation notice and change those api
>> name in the next release.
> This series is not going to be integrated in 17.08.
> Anyway, you should probably send the deprecation notice now,
> in order to change the semantic in 17.11.
> Olivier was also talking about physaddr wording in EAL code.
Per above discussion, we had sent out deprecation notice [1],
and agreed to keep iova patch series on hold for 17.08 release.
Now that v5[2] iova series is reviewed and ready for 17.11.
So iova,v5 series shouldn't be blocked/delayed in case iova deprecation
notice not merged to 17.08 release.
[1] http://dpdk.org/dev/patchwork/patch/26771/
[2] http://dpdk.org/ml/archives/dev/2017-July/071809.html
@@ -78,6 +78,7 @@
#include <rte_ring.h>
#include <rte_memcpy.h>
#include <rte_common.h>
+#include <rte_bus.h>
#ifdef __cplusplus
extern "C" {
@@ -1397,9 +1398,14 @@ rte_mempool_empty(const struct rte_mempool *mp)
static inline phys_addr_t
rte_mempool_virt2phy(__rte_unused const struct rte_mempool *mp, const void *elt)
{
- const struct rte_mempool_objhdr *hdr;
- hdr = (const struct rte_mempool_objhdr *)RTE_PTR_SUB(elt,
+ struct rte_mempool_objhdr *hdr;
+
+ hdr = (struct rte_mempool_objhdr *)RTE_PTR_SUB(elt,
sizeof(*hdr));
+
+ if (rte_eal_iova_mode() == RTE_IOVA_VA)
+ hdr->physaddr = (uintptr_t)elt;
+
return hdr->physaddr;
}