On Fri, Nov 23, 2018 at 11:08:47AM -0600, Parav Pandit wrote:
> Current rxe device counters are not thread safe.
> When multiple QPs are used, they can be racy.
> Make them thread safe by making it per cpu.
>
> Fixes: 0b1e5b99a48b ("IB/rxe: Add port protocol stats")
> Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> ---
> drivers/infiniband/sw/rxe/rxe.c | 17 +++++++++++++++++
> drivers/infiniband/sw/rxe/rxe_hw_counters.c | 17 +++++++++++++++--
> drivers/infiniband/sw/rxe/rxe_verbs.h | 14 ++++++++++----
> 3 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
> index 383e65c..722aca9 100644
> --- a/drivers/infiniband/sw/rxe/rxe.c
> +++ b/drivers/infiniband/sw/rxe/rxe.c
> @@ -39,6 +39,17 @@
> MODULE_DESCRIPTION("Soft RDMA transport");
> MODULE_LICENSE("Dual BSD/GPL");
>
> +static int rxe_init_stats(struct rxe_dev *rxe)
> +{
> + rxe->pcpu_stats = alloc_percpu(struct rxe_stats);
> + return rxe->pcpu_stats ? 0 : -ENOMEM;
> +}
> +
> +static void rxe_cleanup_stats(struct rxe_dev *rxe)
> +{
> + free_percpu(rxe->pcpu_stats);
Just my personal opinion,
I completely dislike this type of abstractions where standard kernel API
is hidden by some fancy one liner, which is called once only.
> +}
> +
> /* free resources for all ports on a device */
> static void rxe_cleanup_ports(struct rxe_dev *rxe)
> {
> @@ -64,6 +75,7 @@ static void rxe_cleanup(struct rxe_dev *rxe)
> rxe_pool_cleanup(&rxe->mc_elem_pool);
>
> rxe_cleanup_ports(rxe);
> + rxe_cleanup_stats(rxe);
>
> crypto_free_shash(rxe->tfm);
> }
> @@ -267,6 +279,10 @@ static int rxe_init(struct rxe_dev *rxe)
> /* init default device parameters */
> rxe_init_device_param(rxe);
>
> + err = rxe_init_stats(rxe);
> + if (err)
> + return err;
> +
> err = rxe_init_ports(rxe);
> if (err)
> goto err1;
> @@ -288,6 +304,7 @@ static int rxe_init(struct rxe_dev *rxe)
> err2:
> rxe_cleanup_ports(rxe);
> err1:
> + rxe_cleanup_stats(rxe);
> return err;
> }
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
> index 6aeb7a1..0fb567a 100644
> --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.c
> +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
> @@ -48,6 +48,19 @@
> [RXE_CNT_SEND_ERR] = "send_err",
> };
>
> +static u64 rxe_stats_read_by_index(const struct rxe_dev *dev, int index)
> +{
> + struct rxe_stats *stats;
> + u64 sum = 0;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + stats = per_cpu_ptr(dev->pcpu_stats, cpu);
> + sum += stats->counters[index];
> + }
> + return sum;
> +}
> +
> int rxe_ib_get_hw_stats(struct ib_device *ibdev,
> struct rdma_hw_stats *stats,
> u8 port, int index)
> @@ -58,8 +71,8 @@ int rxe_ib_get_hw_stats(struct ib_device *ibdev,
> if (!port || !stats)
> return -EINVAL;
>
> - for (cnt = 0; cnt < ARRAY_SIZE(rxe_counter_name); cnt++)
> - stats->value[cnt] = dev->stats_counters[cnt];
> + for (cnt = 0; cnt < ARRAY_SIZE(rxe_counter_name); cnt++)
> + stats->value[cnt] = rxe_stats_read_by_index(dev, cnt);
>
> return ARRAY_SIZE(rxe_counter_name);
> }
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
> index 82e670d..7daff30 100644
> --- a/drivers/infiniband/sw/rxe/rxe_verbs.h
> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
> @@ -380,6 +380,10 @@ struct rxe_port {
> u32 qp_gsi_index;
> };
>
> +struct rxe_stats {
> + u64 counters[RXE_NUM_OF_COUNTERS];
> +};
> +
> struct rxe_dev {
> struct ib_device ib_dev;
> struct ib_device_attr attr;
> @@ -409,16 +413,18 @@ struct rxe_dev {
> spinlock_t mmap_offset_lock; /* guard mmap_offset */
> int mmap_offset;
>
> - u64 stats_counters[RXE_NUM_OF_COUNTERS];
> -
> + struct rxe_stats __percpu *pcpu_stats;
> struct rxe_port port;
> struct list_head list;
> struct crypto_shash *tfm;
> };
>
> -static inline void rxe_counter_inc(struct rxe_dev *rxe, enum rxe_counters cnt)
> +static inline void rxe_counter_inc(struct rxe_dev *rxe, enum rxe_counters index)
> {
> - rxe->stats_counters[cnt]++;
> + struct rxe_stats *stats;
> +
> + stats = this_cpu_ptr(rxe->pcpu_stats);
> + stats->counters[index]++;
> }
>
> static inline struct rxe_dev *to_rdev(struct ib_device *dev)
> --
> 1.8.3.1
>
Attachment:
signature.asc
Description: PGP signature
