[dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime config mmap issue

[dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime config mmap issue

[Li Han]

In rte_eal_config_reattach(),the secondary mmap may fail
due to the rte_mem_cfg_addr already be used by others.do
the change just as the rte_fbarray_init() do.if no
base_virtaddr,use the default 0x100000000.

v2/v3:
-fix code style issues

Signed-off-by: Li Han <han.li1@zte.com.cn>
---
 lib/librte_eal/linux/eal/eal.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index f397206..ab0b5a0 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -308,20 +308,32 @@ enum rte_iova_mode
 {
 	void *rte_mem_cfg_addr;
 	int retval;
+	size_t page_sz, mmap_len;
 
 	const char *pathname = eal_runtime_config_path();
 
 	if (internal_config.no_shconf)
 		return 0;
-
+	mmap_len = sizeof(*rte_config.mem_config);
 	/* map the config before hugepage address so that we don't waste a page */
 	if (internal_config.base_virtaddr != 0)
 		rte_mem_cfg_addr = (void *)
 			RTE_ALIGN_FLOOR(internal_config.base_virtaddr -
 			sizeof(struct rte_mem_config), sysconf(_SC_PAGE_SIZE));
-	else
-		rte_mem_cfg_addr = NULL;
-
+	else {
+		page_sz = sysconf(_SC_PAGESIZE);
+		if (page_sz == (size_t)-1) {
+			RTE_LOG(ERR, EAL,
+				"Cannot get SC_PAGESIZE for rte_mem_config\n");
+			return -1;
+		}
+		rte_mem_cfg_addr = eal_get_virtual_area(NULL, &mmap_len, page_sz, 0, 0);
+		if (rte_mem_cfg_addr == NULL) {
+			RTE_LOG(ERR, EAL,
+				"Cannot get virtual addr for rte_mem_config\n");
+			return -1;
+		}
+	}
 	if (mem_cfg_fd < 0){
 		mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
 		if (mem_cfg_fd < 0) {
@@ -349,8 +361,10 @@ enum rte_iova_mode
 		return -1;
 	}
 
-	rte_mem_cfg_addr = mmap(rte_mem_cfg_addr, sizeof(*rte_config.mem_config),
-				PROT_READ | PROT_WRITE, MAP_SHARED, mem_cfg_fd, 0);
+	rte_mem_cfg_addr = mmap(rte_mem_cfg_addr, mmap_len,
+				PROT_READ | PROT_WRITE,
+				MAP_FIXED | MAP_SHARED,
+				mem_cfg_fd, 0);
 
 	if (rte_mem_cfg_addr == MAP_FAILED){
 		close(mem_cfg_fd);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime config mmap issue

[Burakov, Anatoly]

On 23-Oct-19 11:42 AM, Li Han wrote:
> In rte_eal_config_reattach(),the secondary mmap may fail
> due to the rte_mem_cfg_addr already be used by others.do
> the change just as the rte_fbarray_init() do.if no
> base_virtaddr,use the default 0x100000000.
> 
> v2/v3:
> -fix code style issues
> 
> Signed-off-by: Li Han <han.li1@zte.com.cn>
> ---

Nack. There's a reason why we map it at the same address, and it's to 
have all pointers working across processes. Remapping it at a different 
address has potential to break things.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime config mmap issue

[David Marchand]

On Wed, Oct 23, 2019 at 7:47 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 23-Oct-19 11:42 AM, Li Han wrote:
> > In rte_eal_config_reattach(),the secondary mmap may fail
> > due to the rte_mem_cfg_addr already be used by others.do
> > the change just as the rte_fbarray_init() do.if no
> > base_virtaddr,use the default 0x100000000.
> >
> > v2/v3:
> > -fix code style issues
> >
> > Signed-off-by: Li Han <han.li1@zte.com.cn>
> > ---
>
> Nack. There's a reason why we map it at the same address, and it's to
> have all pointers working across processes. Remapping it at a different
> address has potential to break things.

Marked as rejected.
Thanks.

-- 
David Marchand

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime configmmap issue

[han.li1]

I didn't modified the attach function,I modified the rte_eal_config_create.
    primay process first mmap's address, use a lower address, to avoid
    secondary process mmap the fixed address fail. the secondary still 
    mmap the same address as the primary.
    dpdk do the same thing in rte_fbarray_init() function.


------------------origin------------------
from：DavidMarchand <david.marchand@redhat.com>
to：Burakov, Anatoly <anatoly.burakov@intel.com>;
copy：hanli.li1@zte.com.cn;Thomas Monjalon <thomas@monjalon.net>;dev <dev@dpdk.org>;
date ：2019年10月24日 15:38
title ：Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime configmmap issue
On Wed, Oct 23, 2019 at 7:47 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 23-Oct-19 11:42 AM, Li Han wrote:
> > In rte_eal_config_reattach(),the secondary mmap may fail
> > due to the rte_mem_cfg_addr already be used by others.do
> > the change just as the rte_fbarray_init() do.if no
> > base_virtaddr,use the default 0x100000000.
> >
> > v2/v3:
> > -fix code style issues
> >
> > Signed-off-by: Li Han <han.li1@zte.com.cn>
> > ---
>
> Nack. There's a reason why we map it at the same address, and it's to
> have all pointers working across processes. Remapping it at a different
> address has potential to break things.

Marked as rejected.
Thanks.

--
David Marchand

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime config mmap issue

[Burakov, Anatoly]

On 23-Oct-19 11:42 AM, Li Han wrote:
> In rte_eal_config_reattach(),the secondary mmap may fail
> due to the rte_mem_cfg_addr already be used by others.do
> the change just as the rte_fbarray_init() do.if no
> base_virtaddr,use the default 0x100000000.
> 
> v2/v3:
> -fix code style issues
> 
> Signed-off-by: Li Han <han.li1@zte.com.cn>
> ---

My apologies for earlier NACK, apparently didn't have enough coffee...

I've looked through the patch, and it's a good change.

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

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime config mmap issue

[Burakov, Anatoly]

On 24-Oct-19 8:37 AM, David Marchand wrote:
> On Wed, Oct 23, 2019 at 7:47 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
>>
>> On 23-Oct-19 11:42 AM, Li Han wrote:
>>> In rte_eal_config_reattach(),the secondary mmap may fail
>>> due to the rte_mem_cfg_addr already be used by others.do
>>> the change just as the rte_fbarray_init() do.if no
>>> base_virtaddr,use the default 0x100000000.
>>>
>>> v2/v3:
>>> -fix code style issues
>>>
>>> Signed-off-by: Li Han <han.li1@zte.com.cn>
>>> ---
>>
>> Nack. There's a reason why we map it at the same address, and it's to
>> have all pointers working across processes. Remapping it at a different
>> address has potential to break things.
> 
> Marked as rejected.
> Thanks.
> 

Hi David,

My apologies, I've misinterpreted the intent of the patch. I am 
rescinding my NACK.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime config mmap issue

[David Marchand]

On Thu, Oct 24, 2019 at 1:14 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 24-Oct-19 8:37 AM, David Marchand wrote:
> > On Wed, Oct 23, 2019 at 7:47 PM Burakov, Anatoly
> > <anatoly.burakov@intel.com> wrote:
> >>
> >> On 23-Oct-19 11:42 AM, Li Han wrote:
> >>> In rte_eal_config_reattach(),the secondary mmap may fail
> >>> due to the rte_mem_cfg_addr already be used by others.do
> >>> the change just as the rte_fbarray_init() do.if no
> >>> base_virtaddr,use the default 0x100000000.
> >>>
> >>> v2/v3:
> >>> -fix code style issues
> >>>
> >>> Signed-off-by: Li Han <han.li1@zte.com.cn>
> >>> ---
> >>
> >> Nack. There's a reason why we map it at the same address, and it's to
> >> have all pointers working across processes. Remapping it at a different
> >> address has potential to break things.
> >
> > Marked as rejected.
> > Thanks.
> >
>
> Hi David,
>
> My apologies, I've misinterpreted the intent of the patch. I am
> rescinding my NACK.

Ok, I will put it back in my queue.
No conflict with the work on
http://patchwork.dpdk.org/project/dpdk/list/?series=5854 ?
There was a comment by Stephen, btw.


--
David Marchand

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime config mmap issue

[Burakov, Anatoly]

On 24-Oct-19 12:23 PM, David Marchand wrote:
> On Thu, Oct 24, 2019 at 1:14 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
>>
>> On 24-Oct-19 8:37 AM, David Marchand wrote:
>>> On Wed, Oct 23, 2019 at 7:47 PM Burakov, Anatoly
>>> <anatoly.burakov@intel.com> wrote:
>>>>
>>>> On 23-Oct-19 11:42 AM, Li Han wrote:
>>>>> In rte_eal_config_reattach(),the secondary mmap may fail
>>>>> due to the rte_mem_cfg_addr already be used by others.do
>>>>> the change just as the rte_fbarray_init() do.if no
>>>>> base_virtaddr,use the default 0x100000000.
>>>>>
>>>>> v2/v3:
>>>>> -fix code style issues
>>>>>
>>>>> Signed-off-by: Li Han <han.li1@zte.com.cn>
>>>>> ---
>>>>
>>>> Nack. There's a reason why we map it at the same address, and it's to
>>>> have all pointers working across processes. Remapping it at a different
>>>> address has potential to break things.
>>>
>>> Marked as rejected.
>>> Thanks.
>>>
>>
>> Hi David,
>>
>> My apologies, I've misinterpreted the intent of the patch. I am
>> rescinding my NACK.
> 
> Ok, I will put it back in my queue.
> No conflict with the work on
> http://patchwork.dpdk.org/project/dpdk/list/?series=5854 ?
> There was a comment by Stephen, btw.
> 

Oh, i forgot about that patchset...

Yes, it seems to be doing the same thing, but in a more general way 
(because i was actually fixing an issue on FreeBSD...). Let me get back 
to you on this.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime config mmap issue

[Burakov, Anatoly]

On 24-Oct-19 12:13 PM, Burakov, Anatoly wrote:
> On 23-Oct-19 11:42 AM, Li Han wrote:
>> In rte_eal_config_reattach(),the secondary mmap may fail
>> due to the rte_mem_cfg_addr already be used by others.do
>> the change just as the rte_fbarray_init() do.if no
>> base_virtaddr,use the default 0x100000000.
>>
>> v2/v3:
>> -fix code style issues
>>
>> Signed-off-by: Li Han <han.li1@zte.com.cn>
>> ---
> 
> My apologies for earlier NACK, apparently didn't have enough coffee...
> 
> I've looked through the patch, and it's a good change.
> 
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 

Hi,

It seems that you've basically reimplemented an older patchset [1] that 
i already sent. Could you please test it and see if it fixes your issue 
as well?

[1] http://patchwork.dpdk.org/project/dpdk/list/?series=7043

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime config mmap issue

[Burakov, Anatoly]

On 24-Oct-19 1:22 PM, Burakov, Anatoly wrote:
> On 24-Oct-19 12:23 PM, David Marchand wrote:
>> On Thu, Oct 24, 2019 at 1:14 PM Burakov, Anatoly
>> <anatoly.burakov@intel.com> wrote:
>>>
>>> On 24-Oct-19 8:37 AM, David Marchand wrote:
>>>> On Wed, Oct 23, 2019 at 7:47 PM Burakov, Anatoly
>>>> <anatoly.burakov@intel.com> wrote:
>>>>>
>>>>> On 23-Oct-19 11:42 AM, Li Han wrote:
>>>>>> In rte_eal_config_reattach(),the secondary mmap may fail
>>>>>> due to the rte_mem_cfg_addr already be used by others.do
>>>>>> the change just as the rte_fbarray_init() do.if no
>>>>>> base_virtaddr,use the default 0x100000000.
>>>>>>
>>>>>> v2/v3:
>>>>>> -fix code style issues
>>>>>>
>>>>>> Signed-off-by: Li Han <han.li1@zte.com.cn>
>>>>>> ---
>>>>>
>>>>> Nack. There's a reason why we map it at the same address, and it's to
>>>>> have all pointers working across processes. Remapping it at a 
>>>>> different
>>>>> address has potential to break things.
>>>>
>>>> Marked as rejected.
>>>> Thanks.
>>>>
>>>
>>> Hi David,
>>>
>>> My apologies, I've misinterpreted the intent of the patch. I am
>>> rescinding my NACK.
>>
>> Ok, I will put it back in my queue.
>> No conflict with the work on
>> http://patchwork.dpdk.org/project/dpdk/list/?series=5854 ?
>> There was a comment by Stephen, btw.
>>
> 
> Oh, i forgot about that patchset...
> 
> Yes, it seems to be doing the same thing, but in a more general way 
> (because i was actually fixing an issue on FreeBSD...). Let me get back 
> to you on this.
> 

Yes, that patch is a superset of this one, so this can be discarded, i 
think. Apologies for the back and forth.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime configmmap issue

[han.li1]

it seems your patch is later then mine?
you rejected mine patch and then committed yours?
I can't understand....




------------------origin------------------
from：Burakov,Anatoly <anatoly.burakov@intel.com>
to：David Marchand <david.marchand@redhat.com>;
copy：han.li1;Thomas Monjalon <thomas@monjalon.net>;dev <dev@dpdk.org>;
date ：2019/10/24 20:39
title ：Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime configmmap issue
On 24-Oct-19 1:22 PM, Burakov, Anatoly wrote:
> On 24-Oct-19 12:23 PM, David Marchand wrote:
>> On Thu, Oct 24, 2019 at 1:14 PM Burakov, Anatoly
>> <anatoly.burakov@intel.com> wrote:
>>>
>>> On 24-Oct-19 8:37 AM, David Marchand wrote:
>>>> On Wed, Oct 23, 2019 at 7:47 PM Burakov, Anatoly
>>>> <anatoly.burakov@intel.com> wrote:
>>>>>
>>>>> On 23-Oct-19 11:42 AM, Li Han wrote:
>>>>>> In rte_eal_config_reattach(),the secondary mmap may fail
>>>>>> due to the rte_mem_cfg_addr already be used by others.do
>>>>>> the change just as the rte_fbarray_init() do.if no
>>>>>> base_virtaddr,use the default 0x100000000.
>>>>>>
>>>>>> v2/v3:
>>>>>> -fix code style issues
>>>>>>
>>>>>> Signed-off-by: Li Han <han.li1@zte.com.cn>
>>>>>> ---
>>>>>
>>>>> Nack. There's a reason why we map it at the same address, and it's to
>>>>> have all pointers working across processes. Remapping it at a
>>>>> different
>>>>> address has potential to break things.
>>>>
>>>> Marked as rejected.
>>>> Thanks.
>>>>
>>>
>>> Hi David,
>>>
>>> My apologies, I've misinterpreted the intent of the patch. I am
>>> rescinding my NACK.
>>
>> Ok, I will put it back in my queue.
>> No conflict with the work on
>> http://patchwork.dpdk.org/project/dpdk/list/?series=5854 ?
>> There was a comment by Stephen, btw.
>>
>
> Oh, i forgot about that patchset...
>
> Yes, it seems to be doing the same thing, but in a more general way
> (because i was actually fixing an issue on FreeBSD...). Let me get back
> to you on this.
>

Yes, that patch is a superset of this one, so this can be discarded, i
think. Apologies for the back and forth.

--
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime configmmap issue

[Burakov, Anatoly]

On 25-Oct-19 12:37 PM, han.li1@zte.com.cn wrote:
> it seems your patch is later then mine?
> you rejected mine patch and then committed yours?
> I can't understand....
> 

Hi,

Apologies for the confusion. My patch's v1 was sent in July this year, 
much earlier than yours:

http://patches.dpdk.org/project/dpdk/list/?series=5813&state=%2A&archive=both

And it's a more general patch, that also fixes the issue for FreeBSD. 
This is the patch that got committed - it came in first, and it fixes 
the issue in a more general way, for Linux and for FreeBSD. It's just 
that for some reason i was under the impression that it was already 
accepted, but it was merely deferred till 19.11.

Again, my apologies for the confusion. I should have looked at your 
patch more closely.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime configmmap issue

[David Marchand]

On Fri, Oct 25, 2019 at 2:20 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 25-Oct-19 12:37 PM, han.li1@zte.com.cn wrote:
> > it seems your patch is later then mine?
> > you rejected mine patch and then committed yours?
> > I can't understand....
> >
>
> Hi,
>
> Apologies for the confusion. My patch's v1 was sent in July this year,
> much earlier than yours:
>
> http://patches.dpdk.org/project/dpdk/list/?series=5813&state=%2A&archive=both
>
> And it's a more general patch, that also fixes the issue for FreeBSD.
> This is the patch that got committed - it came in first, and it fixes
> the issue in a more general way, for Linux and for FreeBSD. It's just
> that for some reason i was under the impression that it was already
> accepted, but it was merely deferred till 19.11.
>
> Again, my apologies for the confusion. I should have looked at your
> patch more closely.

I took Anatoly patches since his work on the subject preceded your patch.
Please, test the current master branch and see if you still have issues.

Thanks.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime configmmap issue

[David Marchand]

Hello Li Han,

On Sat, Oct 26, 2019 at 6:07 PM David Marchand
<david.marchand@redhat.com> wrote:
> I took Anatoly patches since his work on the subject preceded your patch.
> Please, test the current master branch and see if you still have issues.

Did you have a chance to test with the -rc1 tag?
Thanks.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v3] lib/librte_eal/linuxapp: fix runtime configmmap issue

[David Marchand]

Hello Li Han,

On Tue, Nov 5, 2019 at 3:14 PM David Marchand <david.marchand@redhat.com> wrote:
> On Sat, Oct 26, 2019 at 6:07 PM David Marchand
> <david.marchand@redhat.com> wrote:
> > I took Anatoly patches since his work on the subject preceded your patch.
> > Please, test the current master branch and see if you still have issues.
>
> Did you have a chance to test with the -rc1 tag?

I will mark this patch as rejected.
If you think it should still be considered, then please set this patch
state back to new in patchwork and explain what is missing in this
thread.

Thanks.


-- 
David Marchand