eal: initialize alarms early
Checks
Commit Message
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
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.
> -----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.
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.
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
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.
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.
> -----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.
@@ -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;