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.
> {
> 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.
--
David Marchand
