Yuanhan, Thank you for the detailed review! Most of your suggestions are very good and I'll fix them in next version.
> -----Original Message----- > From: Yuanhan Liu [mailto:y...@fridaylinux.org] > Sent: Wednesday, September 27, 2017 8:20 PM > To: Tan, Jianfeng > Cc: dev@dpdk.org; Richardson, Bruce; Ananyev, Konstantin; De Lara Guarch, > Pablo; tho...@monjalon.net; maxime.coque...@redhat.com; > mtetsu...@gmail.com; Yigit, Ferruh > Subject: Re: [PATCH 06/12] eal: add channel for primary/secondary > communication > [...] > > +int > > +rte_eal_primary_secondary_add_action(const char *action_name, > > + rte_eal_primary_secondary_t action) > > +{ > > + struct action_entry *entry = malloc(sizeof(struct action_entry)); > > + > > + if (entry == NULL) > > + return -ENOMEM; > > + > > + strncpy(entry->action_name, action_name, > MAX_ACTION_NAME_LEN); > > + entry->action = action; > > + TAILQ_INSERT_TAIL(&action_entry_list, entry, next); > > Since you intended to support "one primary process and multiple secondary > process", here we need a lock to protect the list. Only one thread of each process (either primary or secondary) does the register. So I wonder we don't have to add lock? Of course, no harm to add a lock. > > Another wonder is do we really need that, I mean 1:N model? I'm open to suggestions. IMO, not much extra code for 1:N model than 1:1 model. So not necessary to restrict that. > > > + return 0; > > +} > > + > > +void > > +rte_eal_primary_secondary_del_action(const char *name) > > +{ > > + struct action_entry *entry = find_action_entry_by_name(name); > > + > > + TAILQ_REMOVE(&action_entry_list, entry, next); > > + free(entry); > > +} > > + > > +#define MAX_SECONDARY_PROCS 8 > > + > > +static int efd_pri_sec; /* epoll fd for primary/secondary channel thread */ > > I think it's not a good idea to use "pri". For me, "private" comes to > my mind firstly but not "primary". > > > +static int fd_listen; /* unix listen socket by primary */ > > +static int fd_to_pri; /* only used by secondary process */ > > +static int fds_to_sec[MAX_SECONDARY_PROCS]; > > Too many vars. I'd suggest to use a struct here, which could also make > the naming a bit simpler. For instance, > > struct mp_fds { > int efd; > > union { > /* fds for primary process */ > struct { > int listen; > /* fds used to send msg to secondary process */ > int secondaries[...]; > }; > > /* fds for secondary process */ > struct { > /* fds used to send msg to the primary process */ > int primary; > }; > }; > }; > > It also separates the scope well. Note that above field naming does > not like perfect though. Feel free to come up with some better names. You can always make the code look so clean, thank you! [...] > > +/** Path of primary/secondary communication unix socket file. */ > > +#define PRIMARY_SECONDARY_UNIX_PATH_FMT "%s/.%s_unix" > > +static inline const char * > > +eal_primary_secondary_unix_path(void) > > +{ > > + static char buffer[PATH_MAX]; /* static so auto-zeroed */ > > + const char *directory = default_config_dir; > > + const char *home_dir = getenv("HOME"); > > It's not a good practice to generate such file at HOME dir. User would > be surprised to find it at HOME dir. In the worst case, user might delete > it. This way is the legacy way in DPDK, for example the config path. So I think we should fix that in another patch. > > The more common way is to put it to tmp dir, like "/tmp". Thanks, Jianfeng
