[dpdk-dev] [PATCH 06/12] eal: add channel for primary/secondary communication

[Tan, Jianfeng]

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:yliu at fridaylinux.org]
> Sent: Wednesday, September 27, 2017 8:20 PM
> To: Tan, Jianfeng
> Cc: dev at dpdk.org; Richardson, Bruce; Ananyev, Konstantin; De Lara Guarch,
> Pablo; thomas at monjalon.net; maxime.coquelin at redhat.com;
> mtetsuyah at 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