[v2] timer: allow first subsystem init from secondary
Checks
Commit Message
Since memzones can be reserved from secondary processes as well as
primary processes, if the first call to the timer subsystem init
function occurs in a secondary process, we should allow it to succeed.
Fixes: c0749f7096c7 ("timer: allow management in shared memory")
Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
changes in v2:
- Try to do reserve first, then do lookup on EEXIST (Anatoly)
- Fix uninitialized variable from first version
lib/librte_timer/rte_timer.c | 56 +++++++++++++++++++++++---------------------
1 file changed, 29 insertions(+), 27 deletions(-)
Comments
09/05/2019 21:39, Erik Gabriel Carrillo:
> Since memzones can be reserved from secondary processes as well as
> primary processes, if the first call to the timer subsystem init
> function occurs in a secondary process, we should allow it to succeed.
>
> Fixes: c0749f7096c7 ("timer: allow management in shared memory")
>
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
I think this patch is too big for -rc4.
And it doesn't look so critical.
Do you agree to wait 19.08?
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, May 9, 2019 2:52 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; rsanford@akamai.com;
> dev@dpdk.org
> Subject: Re: [PATCH v2] timer: allow first subsystem init from secondary
>
> 09/05/2019 21:39, Erik Gabriel Carrillo:
> > Since memzones can be reserved from secondary processes as well as
> > primary processes, if the first call to the timer subsystem init
> > function occurs in a secondary process, we should allow it to succeed.
> >
> > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> >
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
>
> I think this patch is too big for -rc4.
> And it doesn't look so critical.
> Do you agree to wait 19.08?
>
The very last hunk of the patch should at least be applied, as it fixes an issue in the finalize() function. The rest of it is just to make sure the behavior is the same as the prior release with respect to the secondary.
I'd prefer if the whole patch was applied, but I can break out the last hunk for a very small patch if that's what you think we should do.
Regards,
Erik
09/05/2019 22:08, Carrillo, Erik G:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 09/05/2019 21:39, Erik Gabriel Carrillo:
> > > Since memzones can be reserved from secondary processes as well as
> > > primary processes, if the first call to the timer subsystem init
> > > function occurs in a secondary process, we should allow it to succeed.
> > >
> > > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> > >
> > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> >
> > I think this patch is too big for -rc4.
> > And it doesn't look so critical.
> > Do you agree to wait 19.08?
> >
>
> The very last hunk of the patch should at least be applied, as it fixes an issue in the finalize() function. The rest of it is just to make sure the behavior is the same as the prior release with respect to the secondary.
>
> I'd prefer if the whole patch was applied, but I can break out the last hunk for a very small patch if that's what you think we should do.
It's a pity it comes so late.
Can you tell how much you think it won't bring any regression?
Are you available everyday until Monday to fix it quickly
if something goes wrong?
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, May 9, 2019 3:13 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; rsanford@akamai.com;
> dev@dpdk.org
> Subject: Re: [PATCH v2] timer: allow first subsystem init from secondary
>
> 09/05/2019 22:08, Carrillo, Erik G:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 09/05/2019 21:39, Erik Gabriel Carrillo:
> > > > Since memzones can be reserved from secondary processes as well as
> > > > primary processes, if the first call to the timer subsystem init
> > > > function occurs in a secondary process, we should allow it to succeed.
> > > >
> > > > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> > > >
> > > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > >
> > > I think this patch is too big for -rc4.
> > > And it doesn't look so critical.
> > > Do you agree to wait 19.08?
> > >
> >
> > The very last hunk of the patch should at least be applied, as it fixes an
> issue in the finalize() function. The rest of it is just to make sure the behavior
> is the same as the prior release with respect to the secondary.
> >
> > I'd prefer if the whole patch was applied, but I can break out the last hunk
> for a very small patch if that's what you think we should do.
>
> It's a pity it comes so late.
>
> Can you tell how much you think it won't bring any regression?
> Are you available everyday until Monday to fix it quickly if something goes
> wrong?
>
I'm fairly sure there won't be a regression; I just did some more testing on the secondary side. But I will be available to fix it if something does go wrong.
Thanks,
Erik
09/05/2019 23:19, Carrillo, Erik G:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 09/05/2019 22:08, Carrillo, Erik G:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 09/05/2019 21:39, Erik Gabriel Carrillo:
> > > > > Since memzones can be reserved from secondary processes as well as
> > > > > primary processes, if the first call to the timer subsystem init
> > > > > function occurs in a secondary process, we should allow it to succeed.
> > > > >
> > > > > Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> > > > >
> > > > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > > >
> > > > I think this patch is too big for -rc4.
> > > > And it doesn't look so critical.
> > > > Do you agree to wait 19.08?
> > > >
> > >
> > > The very last hunk of the patch should at least be applied, as it fixes an
> > issue in the finalize() function. The rest of it is just to make sure the behavior
> > is the same as the prior release with respect to the secondary.
> > >
> > > I'd prefer if the whole patch was applied, but I can break out the last hunk
> > for a very small patch if that's what you think we should do.
> >
> > It's a pity it comes so late.
> >
> > Can you tell how much you think it won't bring any regression?
> > Are you available everyday until Monday to fix it quickly if something goes
> > wrong?
> >
> I'm fairly sure there won't be a regression; I just did some more testing on the secondary side. But I will be available to fix it if something does go wrong.
Applied, thanks
@@ -25,6 +25,7 @@
#include <rte_memzone.h>
#include <rte_malloc.h>
#include <rte_compat.h>
+#include <rte_errno.h>
#include "rte_timer.h"
@@ -155,40 +156,41 @@ rte_timer_subsystem_init_v1905(void)
struct rte_timer_data *data;
int i, lcore_id;
static const char *mz_name = "rte_timer_mz";
+ const size_t data_arr_size =
+ RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
+ bool do_full_init = true;
if (rte_timer_subsystem_initialized)
return -EALREADY;
- if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
- mz = rte_memzone_lookup(mz_name);
- if (mz == NULL)
- return -EEXIST;
-
- rte_timer_data_arr = mz->addr;
-
- rte_timer_data_arr[default_data_id].internal_flags |=
- FL_ALLOCATED;
-
- rte_timer_subsystem_initialized = 1;
-
- return 0;
+reserve:
+ rte_errno = 0;
+ mz = rte_memzone_reserve_aligned(mz_name, data_arr_size, SOCKET_ID_ANY,
+ 0, RTE_CACHE_LINE_SIZE);
+ if (mz == NULL) {
+ if (rte_errno == EEXIST) {
+ mz = rte_memzone_lookup(mz_name);
+ if (mz == NULL)
+ goto reserve;
+
+ do_full_init = false;
+ } else
+ return -ENOMEM;
}
- mz = rte_memzone_reserve_aligned(mz_name,
- RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr),
- SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
- if (mz == NULL)
- return -ENOMEM;
-
rte_timer_data_arr = mz->addr;
- for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
- data = &rte_timer_data_arr[i];
+ if (do_full_init) {
+ for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
+ data = &rte_timer_data_arr[i];
- for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
- rte_spinlock_init(
- &data->priv_timer[lcore_id].list_lock);
- data->priv_timer[lcore_id].prev_lcore = lcore_id;
+ for (lcore_id = 0; lcore_id < RTE_MAX_LCORE;
+ lcore_id++) {
+ rte_spinlock_init(
+ &data->priv_timer[lcore_id].list_lock);
+ data->priv_timer[lcore_id].prev_lcore =
+ lcore_id;
+ }
}
}
@@ -205,8 +207,8 @@ BIND_DEFAULT_SYMBOL(rte_timer_subsystem_init, _v1905, 19.05);
void __rte_experimental
rte_timer_subsystem_finalize(void)
{
- if (rte_timer_data_arr)
- rte_free(rte_timer_data_arr);
+ if (!rte_timer_subsystem_initialized)
+ return;
rte_timer_subsystem_initialized = 0;
}