eal: initialize alarms early

Message ID 20190326184331.13850-1-dariusz.stojaczyk@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • eal: initialize alarms early
Related show

Checks

Context Check Description
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing fail Performance Testing issues
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Stojaczyk, Dariusz March 26, 2019, 6:43 p.m.
We currently initialize rte_alarms after starting
to listen for IPC hotplug requests, which gives
us a data race window. Upon receiving such hotplug
request we always try to set an alarm and this
obviously doesn't work if the alarms weren't
initialized yet.

To fix it, we initialize alarms before starting
to listen for IPC hotplug messages. Specifically,
we move rte_eal_alarm_init() right after
rte_eal_intr_init() as it makes some sense to
keep those two close to each other.

Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
Cc: Qi Zhang <qi.z.zhang@intel.com>
Cc: Anatoly Burakov <anatoly.burakov@intel.com>
Cc: stable@dpdk.org

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
 lib/librte_eal/linux/eal/eal.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Thomas Monjalon March 27, 2019, 6:11 p.m. | #1
26/03/2019 19:43, Darek Stojaczyk:
> We currently initialize rte_alarms after starting
> to listen for IPC hotplug requests, which gives
> us a data race window. Upon receiving such hotplug
> request we always try to set an alarm and this
> obviously doesn't work if the alarms weren't
> initialized yet.
> 
> To fix it, we initialize alarms before starting
> to listen for IPC hotplug messages. Specifically,
> we move rte_eal_alarm_init() right after
> rte_eal_intr_init() as it makes some sense to
> keep those two close to each other.

I wonder which regression it will bring :)
The experience shows that we cannot touch this function
without introducing a regression. Please check twice.

> Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
> Cc: Qi Zhang <qi.z.zhang@intel.com>
> Cc: Anatoly Burakov <anatoly.burakov@intel.com>
> Cc: stable@dpdk.org
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
>  lib/librte_eal/linux/eal/eal.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

You probably need to update the FreeBSD version too.
Stojaczyk, Dariusz March 27, 2019, 8:33 p.m. | #2
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, March 27, 2019 7:12 PM
> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>
> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] eal: initialize alarms early
> 
> 26/03/2019 19:43, Darek Stojaczyk:
> > We currently initialize rte_alarms after starting
> > to listen for IPC hotplug requests, which gives
> > us a data race window. Upon receiving such hotplug
> > request we always try to set an alarm and this
> > obviously doesn't work if the alarms weren't
> > initialized yet.
> >
> > To fix it, we initialize alarms before starting
> > to listen for IPC hotplug messages. Specifically,
> > we move rte_eal_alarm_init() right after
> > rte_eal_intr_init() as it makes some sense to
> > keep those two close to each other.
> 
> I wonder which regression it will bring :)
> The experience shows that we cannot touch this function
> without introducing a regression. Please check twice.

Hah, ok - I'll check again the possible outcomes of this.

> 
> > Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
> > Cc: Qi Zhang <qi.z.zhang@intel.com>
> > Cc: Anatoly Burakov <anatoly.burakov@intel.com>
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > ---
> >  lib/librte_eal/linux/eal/eal.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> You probably need to update the FreeBSD version too.
> 

Oh, that I cannot do. First of all, in bsd code I don't see
rte_mp_dev_hotplug_init() called anywhere, as if bsd
did not listen for IPC hotplug messages at all and hence did
not have any data race in this area. Second, I would be
afraid to touch any bsd code as I'm not running any bsd
system.

D.
Thomas Monjalon March 27, 2019, 10:42 p.m. | #3
27/03/2019 21:33, Stojaczyk, Dariusz:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 26/03/2019 19:43, Darek Stojaczyk:
> > > We currently initialize rte_alarms after starting
> > > to listen for IPC hotplug requests, which gives
> > > us a data race window. Upon receiving such hotplug
> > > request we always try to set an alarm and this
> > > obviously doesn't work if the alarms weren't
> > > initialized yet.
> > >
> > > To fix it, we initialize alarms before starting
> > > to listen for IPC hotplug messages. Specifically,
> > > we move rte_eal_alarm_init() right after
> > > rte_eal_intr_init() as it makes some sense to
> > > keep those two close to each other.
> > 
> > I wonder which regression it will bring :)
> > The experience shows that we cannot touch this function
> > without introducing a regression. Please check twice.
> 
> Hah, ok - I'll check again the possible outcomes of this.
> 
> > 
> > > Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
> > > Cc: Qi Zhang <qi.z.zhang@intel.com>
> > > Cc: Anatoly Burakov <anatoly.burakov@intel.com>
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > > ---
> > >  lib/librte_eal/linux/eal/eal.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > You probably need to update the FreeBSD version too.
> > 
> 
> Oh, that I cannot do. First of all, in bsd code I don't see
> rte_mp_dev_hotplug_init() called anywhere, as if bsd
> did not listen for IPC hotplug messages at all and hence did
> not have any data race in this area. Second, I would be
> afraid to touch any bsd code as I'm not running any bsd
> system.

The problem is the consistency between OSes.
May you ask help here? Bruce is maintaining the FreeBSD side.
Bruce Richardson March 28, 2019, 10:43 a.m. | #4
On Wed, Mar 27, 2019 at 11:42:40PM +0100, Thomas Monjalon wrote:
> 27/03/2019 21:33, Stojaczyk, Dariusz:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 26/03/2019 19:43, Darek Stojaczyk:
> > > > We currently initialize rte_alarms after starting
> > > > to listen for IPC hotplug requests, which gives
> > > > us a data race window. Upon receiving such hotplug
> > > > request we always try to set an alarm and this
> > > > obviously doesn't work if the alarms weren't
> > > > initialized yet.
> > > >
> > > > To fix it, we initialize alarms before starting
> > > > to listen for IPC hotplug messages. Specifically,
> > > > we move rte_eal_alarm_init() right after
> > > > rte_eal_intr_init() as it makes some sense to
> > > > keep those two close to each other.
> > > 
> > > I wonder which regression it will bring :)
> > > The experience shows that we cannot touch this function
> > > without introducing a regression. Please check twice.
> > 
> > Hah, ok - I'll check again the possible outcomes of this.
> > 
> > > 
> > > > Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
> > > > Cc: Qi Zhang <qi.z.zhang@intel.com>
> > > > Cc: Anatoly Burakov <anatoly.burakov@intel.com>
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > > > ---
> > > >  lib/librte_eal/linux/eal/eal.c | 12 ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > You probably need to update the FreeBSD version too.
> > > 
> > 
> > Oh, that I cannot do. First of all, in bsd code I don't see
> > rte_mp_dev_hotplug_init() called anywhere, as if bsd
> > did not listen for IPC hotplug messages at all and hence did
> > not have any data race in this area. Second, I would be
> > afraid to touch any bsd code as I'm not running any bsd
> > system.
> 
> The problem is the consistency between OSes.
> May you ask help here? Bruce is maintaining the FreeBSD side.
> 
I don't think we support IPC or hotplug on BSD, so I don't think this patch
is relevant on the BSD side.

/Bruce
Thomas Monjalon March 28, 2019, 10:46 a.m. | #5
28/03/2019 11:43, Bruce Richardson:
> On Wed, Mar 27, 2019 at 11:42:40PM +0100, Thomas Monjalon wrote:
> > 27/03/2019 21:33, Stojaczyk, Dariusz:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 26/03/2019 19:43, Darek Stojaczyk:
> > > > > We currently initialize rte_alarms after starting
> > > > > to listen for IPC hotplug requests, which gives
> > > > > us a data race window. Upon receiving such hotplug
> > > > > request we always try to set an alarm and this
> > > > > obviously doesn't work if the alarms weren't
> > > > > initialized yet.
> > > > >
> > > > > To fix it, we initialize alarms before starting
> > > > > to listen for IPC hotplug messages. Specifically,
> > > > > we move rte_eal_alarm_init() right after
> > > > > rte_eal_intr_init() as it makes some sense to
> > > > > keep those two close to each other.
> > > > 
> > > > I wonder which regression it will bring :)
> > > > The experience shows that we cannot touch this function
> > > > without introducing a regression. Please check twice.
> > > 
> > > Hah, ok - I'll check again the possible outcomes of this.
> > > 
> > > > 
> > > > > Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
> > > > > Cc: Qi Zhang <qi.z.zhang@intel.com>
> > > > > Cc: Anatoly Burakov <anatoly.burakov@intel.com>
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > > > > ---
> > > > >  lib/librte_eal/linux/eal/eal.c | 12 ++++++------
> > > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > > 
> > > > You probably need to update the FreeBSD version too.
> > > > 
> > > 
> > > Oh, that I cannot do. First of all, in bsd code I don't see
> > > rte_mp_dev_hotplug_init() called anywhere, as if bsd
> > > did not listen for IPC hotplug messages at all and hence did
> > > not have any data race in this area. Second, I would be
> > > afraid to touch any bsd code as I'm not running any bsd
> > > system.
> > 
> > The problem is the consistency between OSes.
> > May you ask help here? Bruce is maintaining the FreeBSD side.
> > 
> I don't think we support IPC or hotplug on BSD, so I don't think this patch
> is relevant on the BSD side.

Yes, but then, the initialization order is different on Linux and BSD.
I'm thinking about consistency and maintenance ease.
Burakov, Anatoly March 28, 2019, 1:14 p.m. | #6
On 28-Mar-19 10:46 AM, Thomas Monjalon wrote:
> 28/03/2019 11:43, Bruce Richardson:
>> On Wed, Mar 27, 2019 at 11:42:40PM +0100, Thomas Monjalon wrote:
>>> 27/03/2019 21:33, Stojaczyk, Dariusz:
>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>>>> 26/03/2019 19:43, Darek Stojaczyk:
>>>>>> We currently initialize rte_alarms after starting
>>>>>> to listen for IPC hotplug requests, which gives
>>>>>> us a data race window. Upon receiving such hotplug
>>>>>> request we always try to set an alarm and this
>>>>>> obviously doesn't work if the alarms weren't
>>>>>> initialized yet.
>>>>>>
>>>>>> To fix it, we initialize alarms before starting
>>>>>> to listen for IPC hotplug messages. Specifically,
>>>>>> we move rte_eal_alarm_init() right after
>>>>>> rte_eal_intr_init() as it makes some sense to
>>>>>> keep those two close to each other.
>>>>>
>>>>> I wonder which regression it will bring :)
>>>>> The experience shows that we cannot touch this function
>>>>> without introducing a regression. Please check twice.
>>>>
>>>> Hah, ok - I'll check again the possible outcomes of this.
>>>>
>>>>>
>>>>>> Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
>>>>>> Cc: Qi Zhang <qi.z.zhang@intel.com>
>>>>>> Cc: Anatoly Burakov <anatoly.burakov@intel.com>
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
>>>>>> ---
>>>>>>   lib/librte_eal/linux/eal/eal.c | 12 ++++++------
>>>>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> You probably need to update the FreeBSD version too.
>>>>>
>>>>
>>>> Oh, that I cannot do. First of all, in bsd code I don't see
>>>> rte_mp_dev_hotplug_init() called anywhere, as if bsd
>>>> did not listen for IPC hotplug messages at all and hence did
>>>> not have any data race in this area. Second, I would be
>>>> afraid to touch any bsd code as I'm not running any bsd
>>>> system.
>>>
>>> The problem is the consistency between OSes.
>>> May you ask help here? Bruce is maintaining the FreeBSD side.
>>>
>> I don't think we support IPC or hotplug on BSD, so I don't think this patch
>> is relevant on the BSD side.
> 
> Yes, but then, the initialization order is different on Linux and BSD.
> I'm thinking about consistency and maintenance ease.
> 

 From my cursory inspection, nothing should be broken on FreeBSD as a 
result of moving alarm API init earlier.
Stojaczyk, Dariusz April 1, 2019, 2:22 p.m. | #7
> -----Original Message-----
> From: Stojaczyk, Dariusz
> Sent: Wednesday, March 27, 2019 9:34 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] eal: initialize alarms early
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Wednesday, March 27, 2019 7:12 PM
> > To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>
> > Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Burakov, Anatoly
> > <anatoly.burakov@intel.com>; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] eal: initialize alarms early
> >
> > 26/03/2019 19:43, Darek Stojaczyk:
> > > We currently initialize rte_alarms after starting
> > > to listen for IPC hotplug requests, which gives
> > > us a data race window. Upon receiving such hotplug
> > > request we always try to set an alarm and this
> > > obviously doesn't work if the alarms weren't
> > > initialized yet.
> > >
> > > To fix it, we initialize alarms before starting
> > > to listen for IPC hotplug messages. Specifically,
> > > we move rte_eal_alarm_init() right after
> > > rte_eal_intr_init() as it makes some sense to
> > > keep those two close to each other.
> >
> > I wonder which regression it will bring :)
> > The experience shows that we cannot touch this function
> > without introducing a regression. Please check twice.
> 
> Hah, ok - I'll check again the possible outcomes of this.
> 

Done, I can't see any case I could break.

> >
> > > Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
> > > Cc: Qi Zhang <qi.z.zhang@intel.com>
> > > Cc: Anatoly Burakov <anatoly.burakov@intel.com>
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > > ---
> > >  lib/librte_eal/linux/eal/eal.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > You probably need to update the FreeBSD version too.
> >
> 
> Oh, that I cannot do. First of all, in bsd code I don't see
> rte_mp_dev_hotplug_init() called anywhere, as if bsd
> did not listen for IPC hotplug messages at all and hence did
> not have any data race in this area. Second, I would be
> afraid to touch any bsd code as I'm not running any bsd
> system.

Basing on Anatoly's feedback, I pushed a v2 with bsd changes
included as well.

Thanks!
D.

> 
> D.

Patch

diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index 13f401684..75ed0cf10 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -1005,6 +1005,12 @@  rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
+	if (rte_eal_alarm_init() < 0) {
+		rte_eal_init_alert("Cannot init interrupt-handling thread");
+		/* rte_eal_alarm_init sets rte_errno on failure. */
+		return -1;
+	}
+
 	/* Put mp channel init before bus scan so that we can init the vdev
 	 * bus through mp channel in the secondary process before the bus scan.
 	 */
@@ -1125,12 +1131,6 @@  rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
-	if (rte_eal_alarm_init() < 0) {
-		rte_eal_init_alert("Cannot init interrupt-handling thread");
-		/* rte_eal_alarm_init sets rte_errno on failure. */
-		return -1;
-	}
-
 	if (rte_eal_timer_init() < 0) {
 		rte_eal_init_alert("Cannot init HPET or TSC timers");
 		rte_errno = ENOTSUP;