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

[Yuanhan Liu <yliu@fridaylinux.org>]

On Thu, Sep 28, 2017 at 01:50:20PM +0000, Tan, Jianfeng wrote:
> > > +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.

Right, I was wrong. I was thinking secondary processes could start at the
same time, that we need a lock here. But as you said, the list is process
specific: each process has it's own copy. No locked is needed then.

> > 
> > 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.

Fair enough. I was just wondering; I have no objection though.

> > > +/** 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.

Yes, I think so.

	--yliu
> 
> > 
> > The more common way is to put it to tmp dir, like "/tmp".
> 
> Thanks,
> Jianfeng