RE: [PATCH dpdk] net/tap: add software MAC address filtering

["Morten Brørup" <mb@smartsharesystems.com>]

> +static inline bool
> +tap_mac_filter_match(struct rx_queue *rxq, struct rte_mbuf *mbuf)
> +{
> +	struct pmd_internals *pmd = rxq->pmd;
> +	struct rte_eth_dev_data *data;
> +	struct rte_ether_addr *dst;
> +	uint32_t i;
> +
> +	if (!pmd->macfilter)
> +		return true;
> +
> +	data = pmd->dev->data;
> +	if (data->promiscuous)
> +		return true;
> +
> +	dst = rte_pktmbuf_mtod(mbuf, struct rte_ether_addr *);
> +
> +	if (rte_is_broadcast_ether_addr(dst))
> +		return true;
> +
> +	if (rte_is_multicast_ether_addr(dst)) {
> +		if (data->all_multicast)
> +			return true;
> +		for (i = 0; i < pmd->nb_mc_addrs; i++) {
> +			if (rte_is_same_ether_addr(dst, &pmd->mc_addrs[i]))
> +				return true;
> +		}
> +		return false;
> +	}
> +
> +	for (i = 0; i < TAP_MAX_MAC_ADDRS; i++) {
> +		if (rte_is_zero_ether_addr(&data->mac_addrs[i]))
> +			continue;
> +		if (rte_is_same_ether_addr(dst, &data->mac_addrs[i]))
> +			return true;
> +	}
> +	return false;
> +}

Performance optimization; the vast majority of packets are unicast:

if (likely(rte_is_unicast_ether_addr(dst)) {
    if (unlikely(rte_is_zero_ether_addr(dst)))
        return false; /* Invalid destination address in packet */
    /* Note: Compiler loop unrolls. */
    for (i = 0; i < TAP_MAX_MAC_ADDRS; i++) {
        if (rte_is_same_ether_addr(dst, &data->mac_addrs[i]))
            return true;
    }
}

if (data->all_multicast)
    return true;

for (i = 0; i < pmd->nb_mc_addrs; i++) {
    if (rte_is_same_ether_addr(dst, &pmd->mc_addrs[i]))
        return true;
}

if (rte_is_broadcast_ether_addr(dst))
    return true;

return false;

Also, all the variables and function parameters could be "const"; although I don't know if it makes any practical difference.


>  static int
> -tap_set_mc_addr_list(struct rte_eth_dev *dev __rte_unused,
> -		     struct rte_ether_addr *mc_addr_set __rte_unused,
> -		     uint32_t nb_mc_addr __rte_unused)
> +tap_set_mc_addr_list(struct rte_eth_dev *dev,
> +		     struct rte_ether_addr *mc_addr_set,
> +		     uint32_t nb_mc_addr)
>  {
> -	/*
> -	 * Nothing to do actually: the tap has no filtering whatsoever,
> every
> -	 * packet is received.
> -	 */
> +	struct pmd_internals *pmd = dev->data->dev_private;
> +
> +	if (nb_mc_addr == 0) {
> +		rte_free(pmd->mc_addrs);
> +		pmd->mc_addrs = NULL;
> +		pmd->nb_mc_addrs = 0;
> +		return 0;
> +	}
> +
> +	pmd->mc_addrs = rte_realloc(pmd->mc_addrs,
> +				    nb_mc_addr * sizeof(*pmd->mc_addrs), 0);
> +	if (pmd->mc_addrs == NULL) {
> +		pmd->nb_mc_addrs = 0;
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(pmd->mc_addrs, mc_addr_set,
> +	       nb_mc_addr * sizeof(*pmd->mc_addrs));
> +	pmd->nb_mc_addrs = nb_mc_addr;
> +
>  	return 0;
>  }

This is not thread safe.
If it needs to be thread safe, you must ensure that tap_mac_filter_match() - running on another lcore - is done using the mc_addrs array before you free it here.