patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] eal: initialize alarms early
@ 2019-03-26 18:43 Darek Stojaczyk
  2019-03-27 18:11 ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
  2019-04-01 14:18 ` [dpdk-stable] [PATCH v2] " Darek Stojaczyk
  0 siblings, 2 replies; 10+ messages in thread
From: Darek Stojaczyk @ 2019-03-26 18:43 UTC (permalink / raw)
  To: dev; +Cc: Darek Stojaczyk, Qi Zhang, Anatoly Burakov, stable

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(-)

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;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: initialize alarms early
  2019-03-26 18:43 [dpdk-stable] [PATCH] eal: initialize alarms early Darek Stojaczyk
@ 2019-03-27 18:11 ` Thomas Monjalon
  2019-03-27 20:33   ` Stojaczyk, Dariusz
  2019-04-01 14:18 ` [dpdk-stable] [PATCH v2] " Darek Stojaczyk
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2019-03-27 18:11 UTC (permalink / raw)
  To: Darek Stojaczyk; +Cc: dev, Qi Zhang, Anatoly Burakov, stable

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.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: initialize alarms early
  2019-03-27 18:11 ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
@ 2019-03-27 20:33   ` Stojaczyk, Dariusz
  2019-03-27 22:42     ` Thomas Monjalon
  2019-04-01 14:22     ` Stojaczyk, Dariusz
  0 siblings, 2 replies; 10+ messages in thread
From: Stojaczyk, Dariusz @ 2019-03-27 20:33 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, Zhang, Qi Z, Burakov, Anatoly, stable



> -----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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: initialize alarms early
  2019-03-27 20:33   ` Stojaczyk, Dariusz
@ 2019-03-27 22:42     ` Thomas Monjalon
  2019-03-28 10:43       ` Bruce Richardson
  2019-04-01 14:22     ` Stojaczyk, Dariusz
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2019-03-27 22:42 UTC (permalink / raw)
  To: Stojaczyk, Dariusz
  Cc: dev, Zhang, Qi Z, Burakov, Anatoly, stable, bruce.richardson

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.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: initialize alarms early
  2019-03-27 22:42     ` Thomas Monjalon
@ 2019-03-28 10:43       ` Bruce Richardson
  2019-03-28 10:46         ` Thomas Monjalon
  0 siblings, 1 reply; 10+ messages in thread
From: Bruce Richardson @ 2019-03-28 10:43 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Stojaczyk, Dariusz, dev, Zhang, Qi Z, Burakov, Anatoly, stable

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: initialize alarms early
  2019-03-28 10:43       ` Bruce Richardson
@ 2019-03-28 10:46         ` Thomas Monjalon
  2019-03-28 13:14           ` Burakov, Anatoly
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2019-03-28 10:46 UTC (permalink / raw)
  To: Bruce Richardson
  Cc: Stojaczyk, Dariusz, dev, Zhang, Qi Z, Burakov, Anatoly, stable

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.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: initialize alarms early
  2019-03-28 10:46         ` Thomas Monjalon
@ 2019-03-28 13:14           ` Burakov, Anatoly
  0 siblings, 0 replies; 10+ messages in thread
From: Burakov, Anatoly @ 2019-03-28 13:14 UTC (permalink / raw)
  To: Thomas Monjalon, Bruce Richardson
  Cc: Stojaczyk, Dariusz, dev, Zhang, Qi Z, stable

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.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-stable] [PATCH v2] eal: initialize alarms early
  2019-03-26 18:43 [dpdk-stable] [PATCH] eal: initialize alarms early Darek Stojaczyk
  2019-03-27 18:11 ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
@ 2019-04-01 14:18 ` Darek Stojaczyk
  2019-04-02 13:01   ` Thomas Monjalon
  1 sibling, 1 reply; 10+ messages in thread
From: Darek Stojaczyk @ 2019-04-01 14:18 UTC (permalink / raw)
  To: dev
  Cc: thomas, bruce.richardson, Darek Stojaczyk, Qi Zhang,
	Anatoly Burakov, stable

On linux, 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.

We update the bsd code as well to keep the initialization
order the same in both eal implementations.

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>
---
v2:
 - updated the bsd code as well (Thomas)


 lib/librte_eal/freebsd/eal/eal.c | 12 ++++++------
 lib/librte_eal/linux/eal/eal.c   | 12 ++++++------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
index 4e86b10b1..790c6afa7 100644
--- a/lib/librte_eal/freebsd/eal/eal.c
+++ b/lib/librte_eal/freebsd/eal/eal.c
@@ -662,6 +662,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.
 	 */
@@ -751,12 +757,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;
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;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: initialize alarms early
  2019-03-27 20:33   ` Stojaczyk, Dariusz
  2019-03-27 22:42     ` Thomas Monjalon
@ 2019-04-01 14:22     ` Stojaczyk, Dariusz
  1 sibling, 0 replies; 10+ messages in thread
From: Stojaczyk, Dariusz @ 2019-04-01 14:22 UTC (permalink / raw)
  To: 'Thomas Monjalon'
  Cc: 'dev@dpdk.org',
	Zhang, Qi Z, Burakov, Anatoly, 'stable@dpdk.org',
	Richardson, Bruce



> -----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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-stable] [PATCH v2] eal: initialize alarms early
  2019-04-01 14:18 ` [dpdk-stable] [PATCH v2] " Darek Stojaczyk
@ 2019-04-02 13:01   ` Thomas Monjalon
  0 siblings, 0 replies; 10+ messages in thread
From: Thomas Monjalon @ 2019-04-02 13:01 UTC (permalink / raw)
  To: Darek Stojaczyk; +Cc: stable, dev, bruce.richardson, Qi Zhang, Anatoly Burakov

01/04/2019 16:18, Darek Stojaczyk:
> On linux, 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.
> 
> We update the bsd code as well to keep the initialization
> order the same in both eal implementations.
> 
> 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>
> ---
> v2:
>  - updated the bsd code as well (Thomas)

Applied, thanks




^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-04-02 13:01 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-26 18:43 [dpdk-stable] [PATCH] eal: initialize alarms early Darek Stojaczyk
2019-03-27 18:11 ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
2019-03-27 20:33   ` Stojaczyk, Dariusz
2019-03-27 22:42     ` Thomas Monjalon
2019-03-28 10:43       ` Bruce Richardson
2019-03-28 10:46         ` Thomas Monjalon
2019-03-28 13:14           ` Burakov, Anatoly
2019-04-01 14:22     ` Stojaczyk, Dariusz
2019-04-01 14:18 ` [dpdk-stable] [PATCH v2] " Darek Stojaczyk
2019-04-02 13:01   ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).