Re: [PATCH v6 1/2] net/iavf: accept up to 32k unicast MAC addresses

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 9/11/2026 1:52 PM, David Marchand wrote:
> On Fri, 11 Sept 2026 at 11:37, Burakov, Anatoly
> <anatoly.burakov@intel.com> 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.burakov@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