Re: [dpdk-dev] [PATCH v3] net/tap: add queues when attaching from secondary process

["Wiles, Keith" <keith.wiles@intel.com>]

> On Jul 20, 2018, at 4:51 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> 20/07/2018 17:35, Wiles, Keith:
>>> On Jul 20, 2018, at 4:15 AM, Thomas Monjalon <thomas@monjalon.net> wrote:
>>> +	/* FIXME: handle replies.nb_received > 1 */
>> 
>> I am not a big fan of having TODO or FIXME comments in the code.
> 
> What don't you like in such comments?

We should not have FIXME or TODO in the code it does not look like it is complete, if we need to fix something then fix it or put it on a todo list not in the code. The same thing for the TODO which to me means a future enhancement we just need to add it to a future todo list.

If the code in these sections have a limitation them describe the limitation and remove the FIXME and TODOs from the code.

> 
>> Can we remove them and just describe the problem and what would happen
>> or not happen if the condition occurs?
> 
> You mean describing the problem in the code?
> 
>> If we need to add this support in the future then we need to put these
>> in a enhancement tracker or someplace else.
> 
> The limitation is documented in the guide (limit of 8 queues).
> 
>>> +	reply = &replies.msgs[0];
> 
> [...]
>>> +	/* FIXME: split message if more queues than RTE_MP_MAX_FD_NUM */
>> 
>> Here too.
> 
> This limitation is related to the previous one (send only one message,
> receive only message).
> 
>>> +	RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM);
>>> +
>>> +	/* Send reply */
>>> +	strcpy(reply.name, request->name);
>>> +	strcpy(reply_param->port_name, request_param->port_name);
>> 
>> Normally we use the snprintf or strlcpy() functions for the above should we do that here too?
> 
> Yes it looks to be a good idea.
> 
> 
>>> @@ -1946,8 +2056,18 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>>> 			TAP_LOG(ERR, "Failed to probe %s", name);
>>> 			return -1;
>>> 		}
>>> -		/* TODO: request info from primary to set up Rx and Tx */
>>> 		eth_dev->dev_ops = &ops;
>>> +		eth_dev->rx_pkt_burst = pmd_rx_burst;
>>> +		eth_dev->tx_pkt_burst = pmd_tx_burst;
>>> +
>>> +		if (!rte_eal_primary_proc_alive(NULL)) {
>>> +			TAP_LOG(ERR, "Primary process is missing");
>>> +			return -1;
>>> +		}
>>> +		ret = tap_mp_attach_queues(name, eth_dev);
>>> +		if (ret != 0)
>>> +			return -1;
>> 
>> Does the call above need to be wrapped using if secondary process or is this for both primary and secondary?
> 
> It is already in a "secondary only" block.
> 
>>> +	/* Register IPC feed callback */
>>> +	ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
>>> +	if (ret < 0 && rte_errno != EEXIST) {
>>> +		TAP_LOG(ERR, "%s: Failed to register IPC callback: %s",
>>> +			tuntap_name, strerror(rte_errno));
>>> +		goto leave;
>>> +	}
>> 
>> Same for this one as above?
> 
> This code path is executed only in primary or creation of port in secondary.
> I think it is fine.
> 
> However I am thinking it should be registered only once for all TAP ports.
> 
> 

Regards,
Keith