Re: [dpdk-dev] [PATCH v7 3/3] net/tap: allow secondary process to access primary device queues

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/17/2018 9:56 AM, Raslan Darawsheh wrote:
> @@ -2082,6 +2214,16 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>  	TAP_LOG(NOTICE, "Initializing pmd_tap for %s as %s",
>  		name, tap_name);
>  
> +	/* Register IPC feed callback */
> +	if (!tap_devices_count) {
> +		ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
> +		if (ret < 0) {
> +			TAP_LOG(ERR, "%s: Failed to register IPC callback: %s",
> +				tuntap_name, strerror(rte_errno));
> +			goto leave;
> +		}
> +	}
> +	tap_devices_count++;
>  	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
>  		ETH_TUNTAP_TYPE_TAP);
>  
> @@ -2089,6 +2231,9 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>  	if (ret == -1) {
>  		TAP_LOG(ERR, "Failed to create pmd for %s as %s",
>  			name, tap_name);
> +		if (!tap_devices_count)
> +			rte_mp_action_unregister(TAP_MP_KEY);
> +		tap_devices_count--;
>  		tap_unit--;		/* Restore the unit number */
>  	}
>  	rte_kvargs_free(kvlist);
Fail recovery part seems broken, it can be like [1] or [2], but both requires a
new variable.
I double checked the logic in prev version of the patch that uses EEXIST return
values, that is also broken. Overall the challenge is in error recovery part we
don't know if we enter there before or after increasing dev_count, that is why a
local variable required.

If you can fix the error recovery path using EEXIST without needing a new
variable, I think that is better, but if not I suggest following [2] since the
logic of increase the dev_count after device successfully created makes sense to
me, but both works.

Thanks,
ferruh


[1]
         /* Register IPC feed callback */
         if (!tap_devices_count) {
                 ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
                 if (ret < 0) {
                         TAP_LOG(ERR, "%s: Failed to register IPC callback: %s",
                                 tuntap_name, strerror(rte_errno));
                         goto leave;
                 }
         }
         tap_devices_count++;
         tap_devices_count_increased = 1;
         ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
                 ETH_TUNTAP_TYPE_TAP);

 leave:
         if (ret == -1) {
                 TAP_LOG(ERR, "Failed to create pmd for %s as %s",
                         name, tap_name);
                 if (tap_devices_count_increased == 1) {
                         if (tap_devices_count == 1)
                                 rte_mp_action_unregister(TAP_MP_KEY);
                         tap_devices_count--;
                 }
                 tap_unit--;             /* Restore the unit number */
         }
         rte_kvargs_free(kvlist);



[2]

         /* Register IPC feed callback */
         if (!tap_devices_count) {
                 ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues);
                 if (ret < 0) {
                         TAP_LOG(ERR, "%s: Failed to register IPC callback: %s",
                                 tuntap_name, strerror(rte_errno));
                         goto leave;
                 }
                 mp_action_registered = 1;
         }
         ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac,
                 ETH_TUNTAP_TYPE_TAP);


 leave:
         if (ret == -1) {
                 TAP_LOG(ERR, "Failed to create pmd for %s as %s",
                         name, tap_name);
                 if (mp_action_registered == 1)
                         rte_mp_action_unregister(TAP_MP_KEY);
                 tap_unit--;             /* Restore the unit number */
         } else {
                 tap_devices_count++;
         }
         rte_kvargs_free(kvlist);