RE: [v11 2/3] net/af_xdp: fix multi interface support for K8s

["Loftus, Ciara" <ciara.loftus@intel.com>]

snip

> @@ -1695,17 +1699,16 @@ xsk_configure(struct pmd_internals *internals,
> struct pkt_rx_queue *rxq,
>  	}
> 
>  	if (internals->use_cni) {
> -		int err, fd, map_fd;
> +		int err, map_fd;
> 
> -		/* get socket fd from CNI plugin */
> -		map_fd = get_cni_fd(internals->if_name);
> +		/* get socket fd from AF_XDP Device Plugin */
> +		map_fd = uds_get_xskmap_fd(internals->if_name, internals-
> >dp_path);
>  		if (map_fd < 0) {
> -			AF_XDP_LOG(ERR, "Failed to receive CNI plugin fd\n");
> +			AF_XDP_LOG(ERR, "Failed to receive xskmap fd from
> AF_XDP Device Plugin\n");
>  			goto out_xsk;
>  		}
> -		/* get socket fd */
> -		fd = xsk_socket__fd(rxq->xsk);
> -		err = bpf_map_update_elem(map_fd, &rxq->xsk_queue_idx,
> &fd, 0);
> +
> +		err = xsk_socket__update_xskmap(rxq->xsk, map_fd);

Hi Maryam,

I've reviewed the series again. I haven't tested the device-plugin specific functionality as I don't have that environment set up, but outside of that I am happy that the new functionality doesn't break anything else. The doc updates look good to me now, thank you for the fixes.

I have just spotted one issue and I apologise for only catching it now.
Patch 2 introduces a dependency on the xsk_socket__update_xskmap function which is available in:
libbpf >= v0.3.0 and <= v0.6.0
libxdp > v1.2.0

The af_xdp.rst guide states we are compatible with libbpf (on it's own) <= v0.6.0. So users using libbpf < v0.3.0 will get an undefined reference warning for the xsk_socket__update_xskmap function.

Is it possible to implement fallback functionality (or if that's not possible, bail out) if that function is not available? See how this is done for the xsk_socket__create_shared function in meson.build and compat.h. 

Thanks,
Ciara

>  		if (err) {
>  			AF_XDP_LOG(ERR, "Failed to insert unprivileged xsk in
> map.\n");
>  			goto out_xsk;
> @@ -1881,13 +1884,13 @@ static const struct eth_dev_ops ops = {
>  	.get_monitor_addr = eth_get_monitor_addr,
>  };
> 
> -/* CNI option works in unprivileged container environment
> - * and ethernet device functionality will be reduced. So
> - * additional customiszed eth_dev_ops struct is needed
> - * for cni. Promiscuous enable and disable functionality
> - * is removed.
> +/* AF_XDP Device Plugin option works in unprivileged
> + * container environments and ethernet device functionality
> + * will be reduced. So additional customised eth_dev_ops
> + * struct is needed for the Device Plugin. Promiscuous
> + * enable and disable functionality is removed.
>   **/
> -static const struct eth_dev_ops ops_cni = {
> +static const struct eth_dev_ops ops_afxdp_dp = {
>  	.dev_start = eth_dev_start,
>  	.dev_stop = eth_dev_stop,
>  	.dev_close = eth_dev_close,
> @@ -2023,7 +2026,8 @@ xdp_get_channels_info(const char *if_name, int
> *max_queues,
>  static int
>  parse_parameters(struct rte_kvargs *kvlist, char *if_name, int *start_queue,
>  		 int *queue_cnt, int *shared_umem, char *prog_path,
> -		 int *busy_budget, int *force_copy, int *use_cni)
> +		 int *busy_budget, int *force_copy, int *use_cni,
> +		 char *dp_path)
>  {
>  	int ret;
> 
> @@ -2069,6 +2073,11 @@ parse_parameters(struct rte_kvargs *kvlist, char
> *if_name, int *start_queue,
>  	if (ret < 0)
>  		goto free_kvlist;
> 
> +	ret = rte_kvargs_process(kvlist, ETH_AF_XDP_DP_PATH_ARG,
> +				 &parse_prog_arg, dp_path);
> +	if (ret < 0)
> +		goto free_kvlist;
> +
>  free_kvlist:
>  	rte_kvargs_free(kvlist);
>  	return ret;
> @@ -2108,7 +2117,7 @@ static struct rte_eth_dev *
>  init_internals(struct rte_vdev_device *dev, const char *if_name,
>  	       int start_queue_idx, int queue_cnt, int shared_umem,
>  	       const char *prog_path, int busy_budget, int force_copy,
> -	       int use_cni)
> +	       int use_cni, const char *dp_path)
>  {
>  	const char *name = rte_vdev_device_name(dev);
>  	const unsigned int numa_node = dev->device.numa_node;
> @@ -2138,6 +2147,7 @@ init_internals(struct rte_vdev_device *dev, const
> char *if_name,
>  	internals->shared_umem = shared_umem;
>  	internals->force_copy = force_copy;
>  	internals->use_cni = use_cni;
> +	strlcpy(internals->dp_path, dp_path, PATH_MAX);
> 
>  	if (xdp_get_channels_info(if_name, &internals->max_queue_cnt,
>  				  &internals->combined_queue_cnt)) {
> @@ -2199,7 +2209,7 @@ init_internals(struct rte_vdev_device *dev, const
> char *if_name,
>  	if (!internals->use_cni)
>  		eth_dev->dev_ops = &ops;
>  	else
> -		eth_dev->dev_ops = &ops_cni;
> +		eth_dev->dev_ops = &ops_afxdp_dp;
> 
>  	eth_dev->rx_pkt_burst = eth_af_xdp_rx;
>  	eth_dev->tx_pkt_burst = eth_af_xdp_tx;
> @@ -2328,6 +2338,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device
> *dev)
>  	int busy_budget = -1, ret;
>  	int force_copy = 0;
>  	int use_cni = 0;
> +	char dp_path[PATH_MAX] = {'\0'};
>  	struct rte_eth_dev *eth_dev = NULL;
>  	const char *name = rte_vdev_device_name(dev);
> 
> @@ -2370,7 +2381,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device
> *dev)
> 
>  	if (parse_parameters(kvlist, if_name, &xsk_start_queue_idx,
>  			     &xsk_queue_cnt, &shared_umem, prog_path,
> -			     &busy_budget, &force_copy, &use_cni) < 0) {
> +			     &busy_budget, &force_copy, &use_cni, dp_path) <
> 0) {
>  		AF_XDP_LOG(ERR, "Invalid kvargs value\n");
>  		return -EINVAL;
>  	}
> @@ -2384,7 +2395,19 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device
> *dev)
>  	if (use_cni && strnlen(prog_path, PATH_MAX)) {
>  		AF_XDP_LOG(ERR, "When '%s' parameter is used, '%s'
> parameter is not valid\n",
>  			ETH_AF_XDP_USE_CNI_ARG,
> ETH_AF_XDP_PROG_ARG);
> -			return -EINVAL;
> +		return -EINVAL;
> +	}
> +
> +	if (use_cni && !strnlen(dp_path, PATH_MAX)) {
> +		snprintf(dp_path, sizeof(dp_path), "%s/%s/%s",
> DP_BASE_PATH, if_name, DP_UDS_SOCK);
> +		AF_XDP_LOG(INFO, "'%s' parameter not provided, setting
> value to '%s'\n",
> +			ETH_AF_XDP_DP_PATH_ARG, dp_path);
> +	}
> +
> +	if (!use_cni && strnlen(dp_path, PATH_MAX)) {
> +		AF_XDP_LOG(ERR, "'%s' parameter is set, but '%s' was not
> enabled\n",
> +			ETH_AF_XDP_DP_PATH_ARG,
> ETH_AF_XDP_USE_CNI_ARG);
> +		return -EINVAL;
>  	}
> 
>  	if (strlen(if_name) == 0) {
> @@ -2410,7 +2433,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device
> *dev)
> 
>  	eth_dev = init_internals(dev, if_name, xsk_start_queue_idx,
>  				 xsk_queue_cnt, shared_umem, prog_path,
> -				 busy_budget, force_copy, use_cni);
> +				 busy_budget, force_copy, use_cni, dp_path);
>  	if (eth_dev == NULL) {
>  		AF_XDP_LOG(ERR, "Failed to init internals\n");
>  		return -1;
> @@ -2471,4 +2494,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_af_xdp,
>  			      "xdp_prog=<string> "
>  			      "busy_budget=<int> "
>  			      "force_copy=<int> "
> -			      "use_cni=<int> ");
> +			      "use_cni=<int> "
> +			      "dp_path=<string> ");
> --
> 2.41.0