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

[Tan, Jianfeng]

Hi Yuanhan,


On 9/29/2017 4:28 PM, Yuanhan Liu wrote:
> On Thu, Sep 28, 2017 at 01:55:59PM +0000, Jianfeng Tan wrote:
>>   static int
>> +share_device(int vid)
>> +{
>> +	uint32_t i, vring_num;
>> +	int len;
>> +	int fds[8];
>> +	struct rte_vhost_memory *mem;
>> +	struct vhost_params *params;
>> +	struct rte_vhost_vring vring;
>> +
>> +	/* share mem table */
>> +	if (rte_vhost_get_mem_table(vid, &mem) < 0) {
>> +		RTE_LOG(ERR, PMD, "Failed to get mem table\n");
>> +		return 0;
>> +	}
>> +	for (i = 0; i < mem->nregions; ++i)
>> +		fds[i] = mem->regions[i].fd;
>> +
>> +	len = sizeof(struct rte_vhost_mem_region) * mem->nregions;
>> +	params = malloc(sizeof(*params) + len);
>> +	if (params == NULL) {
>> +		RTE_LOG(ERR, PMD, "Failed to allocate memory\n");
>> +		return -1;
>> +	}
>> +
>> +	params->type = VHOST_MSG_TYPE_REGIONS;
>> +	params->vid = vid;
>> +	memcpy(params->regions, mem->regions, len);
>> +
>> +	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).

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.

Thanks,
Jianfeng