On Wed, Jun 5, 2019 at 10:50 AM David Marchand <david.march...@redhat.com> wrote:
> > > On Tue, Jun 4, 2019 at 5:45 PM Arnon Warshavsky <ar...@qwilt.com> wrote: > >> This patch changes some void functions to return a value, >> so that the init sequence may tear down orderly >> instead of calling panic. >> > > All we care about in this patch are the panics wrt the shared > configuration init. > Can the commit title and description refer to this ? > Will remove the non-thread from the title. Can you be more specific/ offer your view per the description? > > >> Signed-off-by: Arnon Warshavsky <ar...@qwilt.com> >> --- >> >> The calls for launching core messaging threads were left in tact >> in all 3 eal implementations. >> >> > retval = ftruncate(mem_cfg_fd, sizeof(*rte_config.mem_config)); >> if (retval < 0){ >> close(mem_cfg_fd); >> - rte_panic("Cannot resize '%s' for rte_mem_config\n", >> pathname); >> + mem_cfg_id = -1; >> > > I bet you did not compile FreeBSD :-). > Yup...Not only haste is from the devil..Getting myself a fbsd vm now.. > > + RTE_LOG(ERR, EAL, "Cannot resize '%s' for >> rte_mem_config\n", >> + pathname); >> + return -1; >> } >> >> retval = fcntl(mem_cfg_fd, F_SETLK, &wr_lock); >> if (retval < 0){ >> close(mem_cfg_fd); >> - rte_exit(EXIT_FAILURE, "Cannot create lock on '%s'. Is >> another primary " >> + mem_cfg_fd = -1; >> + RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another >> primary " >> "process running?\n", pathname); >> > > Indent. > Ack > > Not relevant to this patch, but I find it odd to truncate before trying to > take the lock. > Might be something to look at some day, anyway.. :-) > > > + return -1; >> } >> >> rte_mem_cfg_addr = mmap(NULL, sizeof(*rte_config.mem_config), >> PROT_READ | PROT_WRITE, MAP_SHARED, >> mem_cfg_fd, 0); >> >> if (rte_mem_cfg_addr == MAP_FAILED){ >> - rte_panic("Cannot mmap memory for rte_config\n"); >> + RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n"); >> + return -1; >> > > We failed to mmap. > Since eal init is going to fail, we should not leave this fd open. > Ok. In general I tried to stick with previous functionality by just replacing the panic with the exit path, rather than improve it, but this one is indeed straight forward. > >> + } >> } >> >> rte_mem_cfg_addr = mmap(NULL, sizeof(*rte_config.mem_config), >> PROT_READ | PROT_WRITE, MAP_SHARED, >> mem_cfg_fd, 0); >> close(mem_cfg_fd); >> > > At this point, we are in a secondary process. > We don't need to keep this fd open which is fine. > But at least for consistency, we should reset it to -1. > Ack. Same as above > > >> if (rte_mem_cfg_addr == MAP_FAILED){ >> - rte_panic("Cannot mmap memory for rte_config\n"); >> + RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n"); >> + return -1; >> > > Same comment than FreeBSD, mmap failed, we can close/reset mem_cfg_fd. > > Ack > > >> + } >> > > We can close/reset mem_cfg_fd but in the error path _only_: we are going > to reuse it in rte_eal_config_reattach. > > >> rte_config.mem_config = mem_config; >> + >> + return 0; >> } >> >> /* reattach the shared config at exact memory location primary process >> has it */ >> -static void >> +static int >> rte_eal_config_reattach(void) >> { >> struct rte_mem_config *mem_config; >> void *rte_mem_cfg_addr; >> >> if (internal_config.no_shconf) >> - return; >> + return 0; >> >> /* save the address primary process has mapped shared config to */ >> rte_mem_cfg_addr = (void *) (uintptr_t) >> rte_config.mem_config->mem_cfg_addr; >> @@ -403,18 +420,22 @@ enum rte_iova_mode >> sizeof(*mem_config), PROT_READ | PROT_WRITE, >> MAP_SHARED, >> mem_cfg_fd, 0); >> if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) { >> - if (mem_config != MAP_FAILED) >> + if (mem_config != MAP_FAILED) { >> /* errno is stale, don't use */ >> - rte_panic("Cannot mmap memory for rte_config at >> [%p], got [%p]" >> - " - please use '--base-virtaddr' >> option\n", >> - rte_mem_cfg_addr, mem_config); >> - else >> - rte_panic("Cannot mmap memory for rte_config! >> error %i (%s)\n", >> - errno, strerror(errno)); >> + RTE_LOG(ERR, EAL, "Cannot mmap memory for >> rte_config at [%p], got [%p]" >> + " - please use '--base-virtaddr' >> option\n", >> + rte_mem_cfg_addr, mem_config); >> + return -1; >> + } >> + RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! >> error %i (%s)\n", >> + errno, strerror(errno)); >> + return -1; >> } >> close(mem_cfg_fd); >> > > Let's move this close and add a reset to -1 before the branch. > Ack > > >> - rte_config_init(); >> + if (rte_config_init() < 0) { >> + rte_eal_init_alert("Cannot init config"); >> + return -1; >> + } >> > > This hunk is missing for FreeBSD. > > mmmmm , not enough coffee in the process :-/ > thanks