Re: [PATCH rdma-next v1] IB/core: Implement a limit on UMAD receive List

[Mark Zhang <markzhang@nvidia.com>]

On 4/9/2024 9:26 PM, Leon Romanovsky wrote:
> External email: Use caution opening links or attachments
> 
> 
> From: Michael Guralnik <michaelgur@nvidia.com>
> 
> The existing behavior of ib_umad, which maintains received MAD
> packets in an unbounded list, poses a risk of uncontrolled growth.
> As user-space applications extract packets from this list, the rate
> of extraction may not match the rate of incoming packets, leading
> to potential list overflow.
> 
> To address this, we introduce a limit to the size of the list. After
> considering typical scenarios, such as OpenSM processing, which can
> handle approximately 100k packets per second, and the 1-second retry
> timeout for most packets, we set the list size limit to 200k. Packets
> received beyond this limit are dropped, assuming they are likely timed
> out by the time they are handled by user-space.
> 
> Notably, packets queued on the receive list due to reasons like
> timed-out sends are preserved even when the list is full.
> 
> Signed-off-by: Michael Guralnik <michaelgur@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
> Changelog:
> v1:
>   * Changed sysfs entry to hard coded value.
>   * Rewrote the commit message.
> v0: https://lore.kernel.org/all/70029b5f256fbad6efbb98458deb9c46baa2c4b3.1712051390.git.leon@kernel.org
> ---
>   drivers/infiniband/core/user_mad.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
> index f5feca7fa9b9..40756b573017 100644
> --- a/drivers/infiniband/core/user_mad.c
> +++ b/drivers/infiniband/core/user_mad.c
> @@ -63,6 +63,8 @@ MODULE_AUTHOR("Roland Dreier");
>   MODULE_DESCRIPTION("InfiniBand userspace MAD packet access");
>   MODULE_LICENSE("Dual BSD/GPL");
> 
> +#define MAX_UMAD_RECV_LIST_SIZE 200000
> +
>   enum {
>          IB_UMAD_MAX_PORTS  = RDMA_MAX_PORTS,
>          IB_UMAD_MAX_AGENTS = 32,
> @@ -113,6 +115,7 @@ struct ib_umad_file {
>          struct mutex            mutex;
>          struct ib_umad_port    *port;
>          struct list_head        recv_list;
> +       atomic_t                recv_list_size;
>          struct list_head        send_list;
>          struct list_head        port_list;
>          spinlock_t              send_lock;
> @@ -182,7 +185,8 @@ static struct ib_mad_agent *__get_agent(struct ib_umad_file *file, int id)
> 
>   static int queue_packet(struct ib_umad_file *file,
>                          struct ib_mad_agent *agent,
> -                       struct ib_umad_packet *packet)
> +                       struct ib_umad_packet *packet,
> +                       bool is_send_mad)
>   {
>          int ret = 1;
> 
> @@ -192,7 +196,11 @@ static int queue_packet(struct ib_umad_file *file,
>               packet->mad.hdr.id < IB_UMAD_MAX_AGENTS;
>               packet->mad.hdr.id++)
>                  if (agent == __get_agent(file, packet->mad.hdr.id)) {
> +                       if (is_send_mad || atomic_read(&file->recv_list_size) >
> +                                                  MAX_UMAD_RECV_LIST_SIZE)

Should it be:

if (!is_send_mad &&
      atomic_read(&file->recv_list_size) > MAX_UMAD_RECV_LIST_SIZE))

Or maybe:

if (is_recv_mad &&
      atomic_read(&file->recv_list_size) > MAX_UMAD_RECV_LIST_SIZE))

> +                               break;
>                          list_add_tail(&packet->list, &file->recv_list);
> +                       atomic_inc(&file->recv_list_size);
>                          wake_up_interruptible(&file->recv_wait);
>                          ret = 0;
>                          break;
> @@ -224,7 +232,7 @@ static void send_handler(struct ib_mad_agent *agent,
>          if (send_wc->status == IB_WC_RESP_TIMEOUT_ERR) {
>                  packet->length = IB_MGMT_MAD_HDR;
>                  packet->mad.hdr.status = ETIMEDOUT;
> -               if (!queue_packet(file, agent, packet))
> +               if (!queue_packet(file, agent, packet, true))
>                          return;
>          }
>          kfree(packet);
> @@ -284,7 +292,7 @@ static void recv_handler(struct ib_mad_agent *agent,
>                  rdma_destroy_ah_attr(&ah_attr);
>          }
> 
> -       if (queue_packet(file, agent, packet))
> +       if (queue_packet(file, agent, packet, false))
>                  goto err2;
>          return;
> 
> @@ -409,6 +417,7 @@ static ssize_t ib_umad_read(struct file *filp, char __user *buf,
> 
>          packet = list_entry(file->recv_list.next, struct ib_umad_packet, list);
>          list_del(&packet->list);
> +       atomic_dec(&file->recv_list_size);
> 
>          mutex_unlock(&file->mutex);
> 
> @@ -421,6 +430,7 @@ static ssize_t ib_umad_read(struct file *filp, char __user *buf,
>                  /* Requeue packet */
>                  mutex_lock(&file->mutex);
>                  list_add(&packet->list, &file->recv_list);
> +               atomic_inc(&file->recv_list_size);
>                  mutex_unlock(&file->mutex);
>          } else {
>                  if (packet->recv_wc)
> --
> 2.44.0
> 
>