diff mbox series

[v2,5/7] eal: bring forward init of interrupt handling

Message ID 5f6bc60bf193daf4eb07f4484c52878bb1b751de.1530009564.git.anatoly.burakov@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series Remove asynchronous IPC thread |

Checks

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

Commit Message

Burakov, Anatoly June 26, 2018, 10:53 a.m. UTC 
  From: Jianfeng Tan <jianfeng.tan@intel.com>

Next commit will make asynchronous IPC requests rely on alarm API,
which in turn relies on interrupts to work. Therefore, move the EAL
interrupt initialization before IPC initialization to avoid breaking
IPC in the next commit.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/bsdapp/eal/eal.c   | 10 +++++-----
 lib/librte_eal/linuxapp/eal/eal.c | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

Comments

Thomas Monjalon July 12, 2018, 10:36 p.m. UTC | #1 
26/06/2018 12:53, Anatoly Burakov:
> From: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Next commit will make asynchronous IPC requests rely on alarm API,
> which in turn relies on interrupts to work. Therefore, move the EAL
> interrupt initialization before IPC initialization to avoid breaking
> IPC in the next commit.
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -839,6 +839,11 @@ rte_eal_init(int argc, char **argv)
>  
>  	rte_config_init();
>  
> +	if (rte_eal_intr_init() < 0) {
> +		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
> +		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.
>  	 */
> @@ -968,11 +973,6 @@ rte_eal_init(int argc, char **argv)
>  		rte_config.master_lcore, (int)thread_id, cpuset,
>  		ret == 0 ? "" : "...");
>  
> -	if (rte_eal_intr_init() < 0) {
> -		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
> -		return -1;
> -	}
> -
>  	RTE_LCORE_FOREACH_SLAVE(i) {

I am almost sure it will bring regressions.

Please think again about the consequences of initializing interrupt thread
before affinity setting, memory init, device init.

Opinions / ideas from anyone?
Burakov, Anatoly July 13, 2018, 7:41 a.m. UTC | #2 
On 12-Jul-18 11:36 PM, Thomas Monjalon wrote:
> 26/06/2018 12:53, Anatoly Burakov:
>> From: Jianfeng Tan <jianfeng.tan@intel.com>
>>
>> Next commit will make asynchronous IPC requests rely on alarm API,
>> which in turn relies on interrupts to work. Therefore, move the EAL
>> interrupt initialization before IPC initialization to avoid breaking
>> IPC in the next commit.
>>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>> @@ -839,6 +839,11 @@ rte_eal_init(int argc, char **argv)
>>   
>>   	rte_config_init();
>>   
>> +	if (rte_eal_intr_init() < 0) {
>> +		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
>> +		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.
>>   	 */
>> @@ -968,11 +973,6 @@ rte_eal_init(int argc, char **argv)
>>   		rte_config.master_lcore, (int)thread_id, cpuset,
>>   		ret == 0 ? "" : "...");
>>   
>> -	if (rte_eal_intr_init() < 0) {
>> -		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
>> -		return -1;
>> -	}
>> -
>>   	RTE_LCORE_FOREACH_SLAVE(i) {
> 
> I am almost sure it will bring regressions.
> 
> Please think again about the consequences of initializing interrupt thread
> before affinity setting, memory init, device init.
> 
> Opinions / ideas from anyone?
> 

Since interrupt thread is no longer relying on rte_malloc, i do not 
expect there to be any consequences for memory.

I also do not expect any consequences due to moving intr init before 
setting CPU thread affinity, because first of all interrupt thread is 
*already* run before setting thread affinity of the lcores (but after 
setting thread affinity of the master), and it picks its own affinity 
based on parsed coremask anyway, which is already parsed at a point 
where i'm moving it to. Affinities will be set similarly to how they 
were set before, because lcore information is already parsed.

As for device init, that is debatable. The only consequence i can think 
of is if device interrupts happen right after enabling the intr handler 
for that device and this causes some kind of issue or a race. But, we 
already support device hotplug which essentially causes the same 
situation to happen (interrupt handler initialized before bus 
scan/probe), so i'm not really convinced this could have any negative 
consequences either.
David Marchand July 13, 2018, 8:09 a.m. UTC | #3 
On Fri, Jul 13, 2018 at 12:36 AM, Thomas Monjalon <thomas@monjalon.net> wrote:
> I am almost sure it will bring regressions.
>
> Please think again about the consequences of initializing interrupt thread
> before affinity setting, memory init, device init.

Well, there was an issue vith virtio at one time (interrupt handler
did not have the right iopl for virtio callback).
http://git.dpdk.org/dpdk/commit/lib/librte_eal/linuxapp/eal/eal.c?id=fd6949c55c9a48e81c625138679159829d51ac51

Now... time has passed since then.
It is worth checking the issue is still here, and if it is the case,
revisit this.
I suppose virtio pmd should deal with this itself.
Tiwei Bie July 13, 2018, 9:10 a.m. UTC | #4 
On Fri, Jul 13, 2018 at 10:09:42AM +0200, David Marchand wrote:
[...]
> Well, there was an issue vith virtio at one time (interrupt handler
> did not have the right iopl for virtio callback).
> http://git.dpdk.org/dpdk/commit/lib/librte_eal/linuxapp/eal/eal.c?id=fd6949c55c9a48e81c625138679159829d51ac51
> 
> Now... time has passed since then.
> It is worth checking the issue is still here, and if it is the case,
> revisit this.

Now rte_virtio_pmd_init() which calls rte_eal_iopl_init()
is called as a constructor [1], so the issue isn't there
any more.

[1] http://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_ethdev.c?id=1d406458db47#n1789

Best regards,
Tiwei Bie

> I suppose virtio pmd should deal with this itself.
> 
> 
> -- 
> David Marchand
David Marchand July 13, 2018, 11:28 a.m. UTC | #5 
On Fri, Jul 13, 2018 at 11:10 AM, Tiwei Bie <tiwei.bie@intel.com> wrote:
> On Fri, Jul 13, 2018 at 10:09:42AM +0200, David Marchand wrote:
> [...]
>> Well, there was an issue vith virtio at one time (interrupt handler
>> did not have the right iopl for virtio callback).
>> http://git.dpdk.org/dpdk/commit/lib/librte_eal/linuxapp/eal/eal.c?id=fd6949c55c9a48e81c625138679159829d51ac51
>>
>> Now... time has passed since then.
>> It is worth checking the issue is still here, and if it is the case,
>> revisit this.
>
> Now rte_virtio_pmd_init() which calls rte_eal_iopl_init()
> is called as a constructor [1], so the issue isn't there
> any more.

If the virtio pmd is loaded dynamically, eal_plugins_init() is still
called before rte_eal_intr_init().
Yes, for this case, it looks to be fine with Anatoly change.
diff mbox series

Patch

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index dc279542d..f70f7aecd 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -625,6 +625,11 @@  rte_eal_init(int argc, char **argv)
 
 	rte_config_init();
 
+	if (rte_eal_intr_init() < 0) {
+		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
+		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.
 	 */
@@ -713,11 +718,6 @@  rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
-	if (rte_eal_intr_init() < 0) {
-		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
-		return -1;
-	}
-
 	if (rte_eal_timer_init() < 0) {
 		rte_eal_init_alert("Cannot init HPET or TSC timers\n");
 		rte_errno = ENOTSUP;
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 8655b8691..f8a0c06d7 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -839,6 +839,11 @@  rte_eal_init(int argc, char **argv)
 
 	rte_config_init();
 
+	if (rte_eal_intr_init() < 0) {
+		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
+		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.
 	 */
@@ -968,11 +973,6 @@  rte_eal_init(int argc, char **argv)
 		rte_config.master_lcore, (int)thread_id, cpuset,
 		ret == 0 ? "" : "...");
 
-	if (rte_eal_intr_init() < 0) {
-		rte_eal_init_alert("Cannot init interrupt-handling thread\n");
-		return -1;
-	}
-
 	RTE_LCORE_FOREACH_SLAVE(i) {
 
 		/*