From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 82EF3D0C9 for ; Fri, 20 Apr 2018 16:53:24 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B6BAE40704A9; Fri, 20 Apr 2018 14:53:23 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.61]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 108352026DFD; Fri, 20 Apr 2018 14:53:23 +0000 (UTC) From: Aaron Conole To: Arnon Warshavsky Cc: Thomas Monjalon , "Burakov\, Anatoly" , "Lu\, Wenzhuo" , "Doherty\, Declan" , jerin.jacob@caviumnetworks.com, Bruce Richardson , "Yigit\, Ferruh" , dev@dpdk.org, Kevin Traynor References: <1524117669-25729-1-git-send-email-arnon@qwilt.com> <1524117669-25729-11-git-send-email-arnon@qwilt.com> Date: Fri, 20 Apr 2018 10:53:22 -0400 In-Reply-To: (Arnon Warshavsky's message of "Fri, 20 Apr 2018 16:55:49 +0300") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.90 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Fri, 20 Apr 2018 14:53:23 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Fri, 20 Apr 2018 14:53:23 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'aconole@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH v4 10/11] eal: replace rte_panic instances in init sequence X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Apr 2018 14:53:24 -0000 Arnon Warshavsky writes: > Thanks Aaron > > Previously, it wasn't possible for mem_cfg_fd to be reused after a > > failure. Now it is - please reset it to -1. in these close conditions. > > Will do. > > Again, previously this would have aborted on a failure. So it needs to > be reset to a value that allows retry. > > Same here > > > switch (rte_config.process_type){ > > case RTE_PROC_PRIMARY: > > - rte_eal_config_create(); > > + if (rte_eal_config_create()) > > + return -1; > > break; > > case RTE_PROC_SECONDARY: > > - rte_eal_config_attach(); > > + if (rte_eal_config_attach()) > > + return -1; > > rte_eal_mcfg_wait_complete(rte_config.mem_config); > > break; > > case RTE_PROC_AUTO: > > case RTE_PROC_INVALID: > > Not for this patch, but I just noticed that this should probably use a > 'default' case. > > Will add this while Im here > > > Use rte_eal_init_alert to indicate why you are failing the init. > > Will do > > > if (rte_mp_channel_init() < 0) { > > rte_eal_init_alert("failed to init mp channel\n"); > > @@ -652,7 +676,8 @@ static void rte_eal_init_alert(const char *msg) > > > > eal_check_mem_on_local_socket(); > > > > - eal_thread_init_master(rte_config.master_lcore); > > + if (eal_thread_init_master(rte_config.master_lcore) != 0) > > + return -1; > > Is it ever possible to recover from this? > > > Definitely not recoverable, but not different than the other cases where panic propagate all the way > up rather than aborting Looking at the eal_thread_init_master, I think it's probably a recoverable condition. For instance, perhaps the core mask was wrong, and could be corrected by re-attempting the initialization. Just suggesting that it's probably okay to allow a re-attempt here. I would suggest: - eal_thread_init_master(rte_config.master_lcore); + if (eal_thread_init_master(rte_config.master_lcore) != 0) { + rte_eal_init_alert("Cannot assign master lcore\n"); + rte_errno = EINVAL; + return -1; + } if you agree. > Still needs > rte_eal_init_alert() call. > > Will do > > How are you cleaning up the threads that were spawned? Lets say this > loop will execute 5 times, and on the 3rd entry, these errors happen. > You now leave DPDK 'half-initialized' - you've spun up threads and > allocated memory. > > ... > > I don't see how any of this is better for the user. In fact, I think > this is worse because it will make portions of the application stop > working without any way to move forward. rte_panic() will at least give > the process a chance to recover from a potentially ephemeral condition. > > As I wrote in a different reply on this patch > I was probably too eager to get rid of this panic taking some wrong assumptions on the way the > library will be called. Okay - guess emails got crossed in flight :) > Removing the panic from the thread is obviously more complex and also ABI breaking. > From my own bw, I will not make it with a proper change to this version, so I will revert back to > panicing on this patchset > and aim for the thread in the next build. I see. Most likely you'll need a proper initialization protocol both ways. As a brief example, you'll need something to guarantee the thread state (just a general outline): -- global_initial_state = UNINIT pthread_init_cond = PTHREAD_COND_INIT; global_spawned_ctr = atomic_ctr(0); rte_eal_init() ... global_initial_state = INITIALIZING thread_ctr = 0; ... for_each_lcore() spawn_thread() if (spawn_fails) global_initial_state = UNINIT; pthread_cond_broadcast() return error thread_ctr++; while (thread_ctr != global_spawned_ctr) /* wait? figure out when to declare extreme failure */ global_initial_state = THREAD_INITIALIZED pthread_cond_broadcast(pthread_init_cond) while (thread_ctr) wait_some_grace_period() for_each_lcore_thread() thread_state = lcore_state[thr]; /* probably needs a mem barrier*/ if (thread_state != THREAD_READY && thread_state != THREAD_STARTING) /* failure - message all threads to clean up */ if (thread_state == THREAD_READY) thread_ctr--; in eal_thread_loop(): /* before even the set_affinity */ atomic_inc(global_spawned_ctr); lcore_state[thr] = THREAD_STARTING; pthread_cond_wait(pthread_init_cond) if (global_initial_state != THREAD_INITIALIZED) lcore_state[thr] = THREAD_FAILED; return...; /* do all the normal checks... instead of the panic_state, just set lcore_state[thr] to THREAD_FAILED, clean up any additional allocated resources, and return... which will exit the thread */ lcore_state[thr] = THREAD_READY; -- In the above I hope it illustrates what you'll need - a way to signal to each side that initialization phase is happening, and that initialization was successful / failed, and to clean up in the failure case. Just meant for illustration so feel free to ignore / flame, but that's how I would go about removing the rte_panic() calls. > This seems to only exist as a way of triggering the run_once check in > the eal_init. It doesn't add anything except one more state variable to > check against. What is the purpose? > > > Actually this is not a run-once in purpose, rather an attempt to define a state for the device > and on the way work around breaking abi on the the void function called before that. I think it's a way to try and track state for initialization and to prevent future inits. Which ABI are you worried about? rte_panic()? I'm not sure how this is an ABI work around, but I'm probably not thinking about it hard enough. >> + if (rte_get_panic_state()) >> + return -1; >> + > > Please just use run_once. That's a better way of preventing this. > > > As stated above - no a run-once > > All of the comments from the bsd side apply here.