On 9/11/2026 1:52 PM, David Marchand wrote:
On Fri, 11 Sept 2026 at 11:37, Burakov, Anatoly
<[email protected]> wrote:
I would have preferred it if the caller managed the chunking, not the
"add_del_addr_bulk" function. There is precedent for this style of
refactor already [1], and I would like to keep things consistent - keep
the loop simple (without memsets etc.), and make the caller manage how
many addresses are being sent at once.
[1] https://patches.dpdk.org/project/dpdk/
patch/5e6a55afa2b45e3ee5ec17af7a6c548c96e9698b.1771945933.git.anatoly.bura...@intel.com/
This specific refactor is more about removing rte_malloc, but it does
also reorganize the loop in a way that I find to be more readable.
I tried prototyping a loop, and realized that the fact that MAC address
list has holes in it is making things a little difficult, but here's
what I came up with as an alternative implementation, I think it's a
little clearer:
```
#define IAVF_ETH_ADDR_PER_REQ \
((IAVF_AQ_BUF_SZ - sizeof(struct virtchnl_ether_addr_list)) / \
sizeof(struct virtchnl_ether_addr))
struct iavf_eth_addr_cmd {
struct virtchnl_ether_addr_list list;
struct virtchnl_ether_addr extra[IAVF_ETH_ADDR_PER_REQ];
};
static int
iavf_send_uc_addr_list(struct iavf_adapter *adapter,
struct virtchnl_ether_addr_list *list, bool add)
Passing the list object means the function *assumes* that the mac
addresses array follows right after.
Idem, the sending function now assumes the size of the passed object.
If the filling happens at the caller, then I'd rather pass the full
object and its size.
Yes, agreed, although `list` will have information about list size so
IMO just passing the full object is enough.
{
const char *opname = add ? "VIRTCHNL_OP_ADD_ETH_ADDR" :
"VIRTCHNL_OP_DEL_ETH_ADDR";
uint8_t msg_buf[IAVF_AQ_BUF_SZ] = {0};
struct iavf_cmd_info args = {0};
int err;
args.ops = add ? VIRTCHNL_OP_ADD_ETH_ADDR : VIRTCHNL_OP_DEL_ETH_ADDR;
args.in_args = (uint8_t *)list;
args.in_args_size = sizeof(struct virtchnl_ether_addr_list) +
sizeof(struct virtchnl_ether_addr) * list->num_elements;
args.out_buffer = msg_buf;
args.out_size = IAVF_AQ_BUF_SZ;
err = iavf_execute_vf_cmd_safe(adapter, &args);
if (err != 0)
PMD_DRV_LOG(ERR, "fail to execute command %s for %u macs",
opname, list->num_elements);
else
PMD_DRV_LOG(DEBUG, "executed command %s for %u macs",
opname, list->num_elements);
return err;
}
void
iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
{
struct rte_ether_addr *addrs = adapter->dev_data->mac_addrs;
struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
uint32_t idx = 1;
/* Handle primary address (index 0) separately */
if (!rte_is_zero_ether_addr(&addrs[0]))
iavf_add_del_eth_addr(adapter, &addrs[0], add,
VIRTCHNL_ETHER_ADDR_PRIMARY);
/* the secondary address list is sparse, so gather it into full
batches */
while (idx < IAVF_UC_MACADDR_MAX) {
struct iavf_eth_addr_cmd cmd = {0};
uint16_t nb_addrs = 0;
for (; idx < IAVF_UC_MACADDR_MAX && nb_addrs <
IAVF_ETH_ADDR_PER_REQ;
idx++) {
if (rte_is_zero_ether_addr(&addrs[idx]))
continue;
memcpy(cmd.list.list[nb_addrs].addr,
addrs[idx].addr_bytes,
sizeof(cmd.list.list[nb_addrs].addr));
cmd.list.list[nb_addrs].type =
VIRTCHNL_ETHER_ADDR_EXTRA;
nb_addrs++;
}
if (nb_addrs == 0)
break;
cmd.list.vsi_id = vf->vsi_res->vsi_id;
cmd.list.num_elements = nb_addrs;
if (iavf_send_uc_addr_list(adapter, &cmd.list, add) != 0)
break;
}
}
```
Well, if we go with such a refactoring, I am not a fan of the nested
loops, but I get the idea.
I'll have a try.
I would argue that nested loop doing compaction is idiomatic - it's
naturally a nested loop operation, so we're going to have nested loops
either way. However, the control flow is IMO much cleaner that way,
because there is no special casing inside the "send the list" function,
and additionally, such an approach lends itself to much fewer virtchnl
calls - with your code, in a degenerate "valid addr in every other
slot", you'd essentially be spamming virtchnl on every addr, while with
a nested loop like mine, you'd just compact it into a list straight away
and get away with far fewer virtchnl call-ins. So, I'd really like to
keep this kind of flow, if you don't mind :)
--
Thanks,
Anatoly
