Re: [PATCH iproute2-next 0/4] Introduce FRMR pools

[David Ahern <dsahern@gmail.com>]

On 3/2/26 8:51 AM, Chiara Meiohas wrote:
> From Michael:
> 
> This series adds support for managing Fast Registration Memory Region
> (FRMR) pools in rdma tool, enabling users to monitor and configure FRMR
> pool behavior.
> 

Claude has some quibbles with the patches:


1. Type mismatch: RDMA_NLDEV_ATTR_RES_FRMR_POOL_AGING_PERIOD

In rdma/include/uapi/rdma/rdma_netlink.h:
> +     RDMA_NLDEV_ATTR_RES_FRMR_POOL_AGING_PERIOD,     /* u64 */

In rdma/res-frmr-pools.c (res_frmr_pools_one_set_aging):
> +     uint32_t aging_period;
> +     mnl_attr_put_u32(rd->nlh,
RDMA_NLDEV_ATTR_RES_FRMR_POOL_AGING_PERIOD,
> +                      aging_period);

The uapi header documents this attribute as u64, but the code declares a
uint32_t variable and sends it with mnl_attr_put_u32(). This mismatch
means userspace will send a 4-byte attribute when the kernel expects 8
bytes, leading to either a parse error or silent data truncation.

Fix: use uint64_t and mnl_attr_put_u64() to match the uapi annotation,
or correct the uapi comment to reflect the intended wire type.

---

2. Type mismatch: RDMA_NLDEV_ATTR_RES_FRMR_POOL_PINNED

In rdma/include/uapi/rdma/rdma_netlink.h:
> +     RDMA_NLDEV_ATTR_RES_FRMR_POOL_PINNED,           /* u8 */

In rdma/res-frmr-pools.c (res_frmr_pools_line):
> +     uint32_t queue_handles = 0, pinned_handles = 0;
> +     if (nla_line[RDMA_NLDEV_ATTR_RES_FRMR_POOL_PINNED])
> +             pinned_handles = mnl_attr_get_u32(
> +                     nla_line[RDMA_NLDEV_ATTR_RES_FRMR_POOL_PINNED]);

In rdma/res-frmr-pools.c (res_frmr_pools_one_set_pinned):
> +     uint32_t pinned_value;
> +     mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_RES_FRMR_POOL_PINNED,
> +                      pinned_value);

The uapi header marks this as u8, but the code reads and writes it as u32
using mnl_attr_get_u32() and mnl_attr_put_u32(). The type used in
userspace must match the kernel's attribute definition.

Fix: decide on the actual wire type and make the uapi comment, variable
declaration, and mnl accessor consistent. If u32 is correct, change the
comment to /* u32 */; if u8 is correct, use uint8_t and mnl_attr_put_u8().

---

3. Declared but unimplemented: res_frmr_pools_idx_parse_cb

In rdma/res.h:
> +int res_frmr_pools_idx_parse_cb(const struct nlmsghdr *nlh, void *data);

This function is declared alongside all the other *_idx_parse_cb symbols,
but there is no corresponding definition in rdma/res-frmr-pools.c.

Because res_frmr_pools uses id=0 in RES_FUNC:
> +RES_FUNC(res_frmr_pools, RDMA_NLDEV_CMD_RES_FRMR_POOLS_GET,
> +      frmr_pools_valid_filters, true, 0);

the macro's idx path is never triggered, so this won't cause a link error
in practice. However, the declaration is misleading and inconsistent with
the other resource types where the idx callback exists because they pass a
non-zero id. Either implement the function (if per-index lookup is
planned) or remove the declaration from res.h.

---