[dpdk-dev] [PATCH v2 12/12] net/vhost: support to run in the secondary process

[Tan, Jianfeng]

On 9/30/2017 4:16 PM, Yuanhan Liu wrote:
> On Sat, Sep 30, 2017 at 12:03:33PM +0800, Tan, Jianfeng wrote:
>>>> +	if (rte_eal_mp_sendmsg("vhost pmd", params, sizeof(*params) + len,
>>> To me, it's not a good idea to identify an object by a string. The common
>>> practice is to use a handler, which could either be a struct or a nubmer.
>> My point is that if we choose the way you suggested, we need somewhere to
>> maintain them. For example, every time we need register a new action, we
>> need to update that file to add a new entry (number).
> Not really. You could let the register function to return a struct, in
> which there is a name field.
>
> Anyway, I think it's not a big deal to turn it to struct, judging that
> it may only contain one field only: the name. But at least, you should
> not type the same string everywhere. Use macro instead.

Agreed. Will fix that.

>
>> In current way, the duplicated register with the same name will fail,
>> developers is responsible to check that.
>>
>>>> +			       fds, mem->nregions) < 0) {
>>>> +		RTE_LOG(ERR, PMD, "Failed to share mem table\n");
>>>> +		free(params);
>>>> +		return -1;
>>>> +	}
>>>> +
>>>> +	/* share callfd and kickfd */
>>>> +	params->type = VHOST_MSG_TYPE_SET_FDS;
>>>> +	vring_num = rte_vhost_get_vring_num(vid);
>>>> +	for (i = 0; i < vring_num; i++) {
>>>> +		if (rte_vhost_get_vhost_vring(vid, i, &vring) < 0) {
>>> If you save the fds here, you don't have to get it every time when there
>>> is a new secondary process attached. Then as I have suggested firstly,
>>> you don't have to introduce callfd_pri in the vhost lib.
>> If we don't introduce callfd_pri, when we do virtqueue cleanup (or similar
>> operations) in vhost lib, it will wrongly close fds belonging to secondary
>> process.
>>
>> You remind me that, instead of introduce callfd_pri, we can introduce
>> callfd_effective, to reduce the modification lines.
> It's not about how many lines are modified. You were adding "effective"
> fds, which is a semantic change. It makes the logic a bit more complex.
> What's worse, it even doesn't resolve the issue completely.

Yes, it still has limitation. Just wonder if possible to move event fd 
write out of vhost lib.

Thanks,
Jianfeng

>
> 	--yliu