diff mbox series

[2/2] eal/freebsd: add config reattach

Message ID 0fdd08d79feb1b60c304a1194d14282238cb679e.1561477829.git.anatoly.burakov@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [1/2] eal/freebsd: fix missing write to internal config |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Burakov, Anatoly June 25, 2019, 3:50 p.m. UTC 
  Linux EAL will attach the shared config at an arbitrary address,
find out where the shared config is mapped in the primary, and
then will reattach it at that exact address.

FreeBSD version doesn't seem to go for that extra reattach step,
which makes one wonder how did it ever work in the first place.

Fix the FreeBSD init to also reattach shared config to the exact
same place the primary process has it.

Fixes: 764bf26873b9 ("add FreeBSD support")
Cc: bruce.richardson@intel.com
Cc: stable@dpdk.org

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/freebsd/eal/eal.c | 36 ++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

David Marchand June 26, 2019, 12:03 p.m. UTC | #1 
On Tue, Jun 25, 2019 at 5:51 PM Anatoly Burakov <anatoly.burakov@intel.com>
wrote:

> Linux EAL will attach the shared config at an arbitrary address,
> find out where the shared config is mapped in the primary, and
> then will reattach it at that exact address.
>
> FreeBSD version doesn't seem to go for that extra reattach step,
> which makes one wonder how did it ever work in the first place.
>
> Fix the FreeBSD init to also reattach shared config to the exact
> same place the primary process has it.
>
> Fixes: 764bf26873b9 ("add FreeBSD support")
> Cc: bruce.richardson@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/freebsd/eal/eal.c | 36 ++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/lib/librte_eal/freebsd/eal/eal.c
> b/lib/librte_eal/freebsd/eal/eal.c
> index 8c399c799..ce7a5f91d 100644
> --- a/lib/librte_eal/freebsd/eal/eal.c
> +++ b/lib/librte_eal/freebsd/eal/eal.c
> @@ -280,6 +280,41 @@ rte_eal_config_attach(void)
>         rte_config.mem_config = rte_mem_cfg_addr;
>  }
>
> +/* reattach the shared config at exact memory location primary process
> has it */
> +static void
> +rte_eal_config_reattach(void)
> +{
> +       struct rte_mem_config *mem_config;
> +       void *rte_mem_cfg_addr;
> +
> +       if (internal_config.no_shconf)
> +               return;
> +
> +       /* save the address primary process has mapped shared config to */
> +       rte_mem_cfg_addr =
> +                       (void
> *)(uintptr_t)rte_config.mem_config->mem_cfg_addr;
>

It should be within the 80 columns limit on a single line.

+
> +       /* unmap original config */
> +       munmap(rte_config.mem_config, sizeof(struct rte_mem_config));
>

Hum, the previous mapping is PROT_WRITE while unneeded, right?


+
> +       /* remap the config at proper address */
> +       mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
> +                       sizeof(*mem_config), PROT_READ | PROT_WRITE,
> MAP_SHARED,
> +                       mem_cfg_fd, 0);
>

mem_cfg_fd should have been closed in rte_eal_config_attach().
So it should fail here.


+       if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
> +               if (mem_config != MAP_FAILED)
> +                       /* errno is stale, don't use */
> +                       rte_panic("Cannot mmap memory for rte_config at
> [%p], got [%p]\n",
> +                                 rte_mem_cfg_addr, mem_config);
> +               else
> +                       rte_panic("Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                                 errno, strerror(errno));
> +       }
> +       close(mem_cfg_fd);
> +
> +       rte_config.mem_config = mem_config;
> +}
> +
>  /* Detect if we are a primary or a secondary process */
>  enum rte_proc_type_t
>  eal_proc_type_detect(void)
> @@ -318,6 +353,7 @@ rte_config_init(void)
>         case RTE_PROC_SECONDARY:
>                 rte_eal_config_attach();
>                 rte_eal_mcfg_wait_complete(rte_config.mem_config);
> +               rte_eal_config_reattach();
>                 break;
>         case RTE_PROC_AUTO:
>         case RTE_PROC_INVALID:
> --
> 2.17.1
>


Is there a reason why FreeBSD EAL does not support the virtaddr hint like
for Linux EAL?
David Marchand June 26, 2019, 12:21 p.m. UTC | #2 
On Wed, Jun 26, 2019 at 2:03 PM David Marchand <david.marchand@redhat.com>
wrote:

>
>
> On Tue, Jun 25, 2019 at 5:51 PM Anatoly Burakov <anatoly.burakov@intel.com>
> wrote:
>
>> Linux EAL will attach the shared config at an arbitrary address,
>> find out where the shared config is mapped in the primary, and
>> then will reattach it at that exact address.
>>
>> FreeBSD version doesn't seem to go for that extra reattach step,
>> which makes one wonder how did it ever work in the first place.
>>
>> Fix the FreeBSD init to also reattach shared config to the exact
>> same place the primary process has it.
>>
>> Fixes: 764bf26873b9 ("add FreeBSD support")
>> Cc: bruce.richardson@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>  lib/librte_eal/freebsd/eal/eal.c | 36 ++++++++++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/lib/librte_eal/freebsd/eal/eal.c
>> b/lib/librte_eal/freebsd/eal/eal.c
>> index 8c399c799..ce7a5f91d 100644
>> --- a/lib/librte_eal/freebsd/eal/eal.c
>> +++ b/lib/librte_eal/freebsd/eal/eal.c
>> @@ -280,6 +280,41 @@ rte_eal_config_attach(void)
>>         rte_config.mem_config = rte_mem_cfg_addr;
>>  }
>>
>> +/* reattach the shared config at exact memory location primary process
>> has it */
>> +static void
>> +rte_eal_config_reattach(void)
>> +{
>> +       struct rte_mem_config *mem_config;
>> +       void *rte_mem_cfg_addr;
>> +
>> +       if (internal_config.no_shconf)
>> +               return;
>> +
>> +       /* save the address primary process has mapped shared config to */
>> +       rte_mem_cfg_addr =
>> +                       (void
>> *)(uintptr_t)rte_config.mem_config->mem_cfg_addr;
>>
>
> It should be within the 80 columns limit on a single line.
>
> +
>> +       /* unmap original config */
>> +       munmap(rte_config.mem_config, sizeof(struct rte_mem_config));
>>
>
> Hum, the previous mapping is PROT_WRITE while unneeded, right?
>
>
> +
>> +       /* remap the config at proper address */
>> +       mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
>> +                       sizeof(*mem_config), PROT_READ | PROT_WRITE,
>> MAP_SHARED,
>> +                       mem_cfg_fd, 0);
>>
>
> mem_cfg_fd should have been closed in rte_eal_config_attach().
> So it should fail here.
>
>
> +       if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
>> +               if (mem_config != MAP_FAILED)
>> +                       /* errno is stale, don't use */
>> +                       rte_panic("Cannot mmap memory for rte_config at
>> [%p], got [%p]\n",
>> +                                 rte_mem_cfg_addr, mem_config);
>> +               else
>> +                       rte_panic("Cannot mmap memory for rte_config!
>> error %i (%s)\n",
>> +                                 errno, strerror(errno));
>> +       }
>> +       close(mem_cfg_fd);
>> +
>> +       rte_config.mem_config = mem_config;
>> +}
>> +
>>  /* Detect if we are a primary or a secondary process */
>>  enum rte_proc_type_t
>>  eal_proc_type_detect(void)
>> @@ -318,6 +353,7 @@ rte_config_init(void)
>>         case RTE_PROC_SECONDARY:
>>                 rte_eal_config_attach();
>>                 rte_eal_mcfg_wait_complete(rte_config.mem_config);
>> +               rte_eal_config_reattach();
>>                 break;
>>         case RTE_PROC_AUTO:
>>         case RTE_PROC_INVALID:
>> --
>> 2.17.1
>>
>
>
> Is there a reason why FreeBSD EAL does not support the virtaddr hint like
> for Linux EAL?
>

Not sure you noticed, but Arnon also had sent a patch touching the config
mapping parts.
http://patchwork.dpdk.org/patch/54583/
Burakov, Anatoly June 26, 2019, 12:49 p.m. UTC | #3 
On 26-Jun-19 1:03 PM, David Marchand wrote:
> On Tue, Jun 25, 2019 at 5:51 PM Anatoly Burakov <anatoly.burakov@intel.com>
> wrote:
> 
>> Linux EAL will attach the shared config at an arbitrary address,
>> find out where the shared config is mapped in the primary, and
>> then will reattach it at that exact address.
>>
>> FreeBSD version doesn't seem to go for that extra reattach step,
>> which makes one wonder how did it ever work in the first place.
>>
>> Fix the FreeBSD init to also reattach shared config to the exact
>> same place the primary process has it.
>>
>> Fixes: 764bf26873b9 ("add FreeBSD support")
>> Cc: bruce.richardson@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_eal/freebsd/eal/eal.c | 36 ++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/lib/librte_eal/freebsd/eal/eal.c
>> b/lib/librte_eal/freebsd/eal/eal.c
>> index 8c399c799..ce7a5f91d 100644
>> --- a/lib/librte_eal/freebsd/eal/eal.c
>> +++ b/lib/librte_eal/freebsd/eal/eal.c
>> @@ -280,6 +280,41 @@ rte_eal_config_attach(void)
>>          rte_config.mem_config = rte_mem_cfg_addr;
>>   }
>>
>> +/* reattach the shared config at exact memory location primary process
>> has it */
>> +static void
>> +rte_eal_config_reattach(void)
>> +{
>> +       struct rte_mem_config *mem_config;
>> +       void *rte_mem_cfg_addr;
>> +
>> +       if (internal_config.no_shconf)
>> +               return;
>> +
>> +       /* save the address primary process has mapped shared config to */
>> +       rte_mem_cfg_addr =
>> +                       (void
>> *)(uintptr_t)rte_config.mem_config->mem_cfg_addr;
>>
> 
> It should be within the 80 columns limit on a single line.

Nope, tried it - doesn't fit.

> 
> +
>> +       /* unmap original config */
>> +       munmap(rte_config.mem_config, sizeof(struct rte_mem_config));
>>
> 
> Hum, the previous mapping is PROT_WRITE while unneeded, right?

Yes, good catch.

> 
> 
> +
>> +       /* remap the config at proper address */
>> +       mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
>> +                       sizeof(*mem_config), PROT_READ | PROT_WRITE,
>> MAP_SHARED,
>> +                       mem_cfg_fd, 0);
>>
> 
> mem_cfg_fd should have been closed in rte_eal_config_attach().
> So it should fail here.

Hmm, wonder why it didn't. I'll investigate and fix if necessary.

> 
> 
> +       if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
>> +               if (mem_config != MAP_FAILED)
>> +                       /* errno is stale, don't use */
>> +                       rte_panic("Cannot mmap memory for rte_config at
>> [%p], got [%p]\n",
>> +                                 rte_mem_cfg_addr, mem_config);
>> +               else
>> +                       rte_panic("Cannot mmap memory for rte_config!
>> error %i (%s)\n",
>> +                                 errno, strerror(errno));
>> +       }
>> +       close(mem_cfg_fd);
>> +
>> +       rte_config.mem_config = mem_config;
>> +}
>> +
>>   /* Detect if we are a primary or a secondary process */
>>   enum rte_proc_type_t
>>   eal_proc_type_detect(void)
>> @@ -318,6 +353,7 @@ rte_config_init(void)
>>          case RTE_PROC_SECONDARY:
>>                  rte_eal_config_attach();
>>                  rte_eal_mcfg_wait_complete(rte_config.mem_config);
>> +               rte_eal_config_reattach();
>>                  break;
>>          case RTE_PROC_AUTO:
>>          case RTE_PROC_INVALID:
>> --
>> 2.17.1
>>
> 
> 
> Is there a reason why FreeBSD EAL does not support the virtaddr hint like
> for Linux EAL?
> 

No real reason. Implementing that is probably out of scope for this 
patchset though.
Burakov, Anatoly June 26, 2019, 12:50 p.m. UTC | #4 
On 26-Jun-19 1:21 PM, David Marchand wrote:
> On Wed, Jun 26, 2019 at 2:03 PM David Marchand <david.marchand@redhat.com>
> wrote:
> 
>>
>>
>> On Tue, Jun 25, 2019 at 5:51 PM Anatoly Burakov <anatoly.burakov@intel.com>
>> wrote:
>>
>>> Linux EAL will attach the shared config at an arbitrary address,
>>> find out where the shared config is mapped in the primary, and
>>> then will reattach it at that exact address.
>>>
>>> FreeBSD version doesn't seem to go for that extra reattach step,
>>> which makes one wonder how did it ever work in the first place.
>>>
>>> Fix the FreeBSD init to also reattach shared config to the exact
>>> same place the primary process has it.
>>>
>>> Fixes: 764bf26873b9 ("add FreeBSD support")
>>> Cc: bruce.richardson@intel.com
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>> ---

<...>

> Not sure you noticed, but Arnon also had sent a patch touching the config
> mapping parts.
> http://patchwork.dpdk.org/patch/54583/
> 

Yes, i've seen those, didn't get a chance to look at them yet. I'll 
rebase when necessary.
diff mbox series

Patch

diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
index 8c399c799..ce7a5f91d 100644
--- a/lib/librte_eal/freebsd/eal/eal.c
+++ b/lib/librte_eal/freebsd/eal/eal.c
@@ -280,6 +280,41 @@  rte_eal_config_attach(void)
 	rte_config.mem_config = rte_mem_cfg_addr;
 }
 
+/* reattach the shared config at exact memory location primary process has it */
+static void
+rte_eal_config_reattach(void)
+{
+	struct rte_mem_config *mem_config;
+	void *rte_mem_cfg_addr;
+
+	if (internal_config.no_shconf)
+		return;
+
+	/* save the address primary process has mapped shared config to */
+	rte_mem_cfg_addr =
+			(void *)(uintptr_t)rte_config.mem_config->mem_cfg_addr;
+
+	/* unmap original config */
+	munmap(rte_config.mem_config, sizeof(struct rte_mem_config));
+
+	/* remap the config at proper address */
+	mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
+			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
+			mem_cfg_fd, 0);
+	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
+		if (mem_config != MAP_FAILED)
+			/* errno is stale, don't use */
+			rte_panic("Cannot mmap memory for rte_config at [%p], got [%p]\n",
+				  rte_mem_cfg_addr, mem_config);
+		else
+			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
+				  errno, strerror(errno));
+	}
+	close(mem_cfg_fd);
+
+	rte_config.mem_config = mem_config;
+}
+
 /* Detect if we are a primary or a secondary process */
 enum rte_proc_type_t
 eal_proc_type_detect(void)
@@ -318,6 +353,7 @@  rte_config_init(void)
 	case RTE_PROC_SECONDARY:
 		rte_eal_config_attach();
 		rte_eal_mcfg_wait_complete(rte_config.mem_config);
+		rte_eal_config_reattach();
 		break;
 	case RTE_PROC_AUTO:
 	case RTE_PROC_INVALID: