diff mbox series

[rdma-next,v1,04/11] RDMA/counter: Add an is_disabled field in struct rdma_hw_stats

Message ID 97ef07eab2b56b39f3c93e12364237d5aec2acb6.1631660727.git.leonro@nvidia.com (mailing list archive)
State Superseded
Delegated to: Jason Gunthorpe
Headers show
Series Optional counter statistics support | expand

Commit Message

Leon Romanovsky Sept. 14, 2021, 11:07 p.m. UTC 
From: Aharon Landau <aharonl@nvidia.com>

Add a bitmap in rdma_hw_stat structure, with each bit indicates whether
the corresponding counter is currently disabled or not. By default
hwcounters are enabled.

Signed-off-by: Aharon Landau <aharonl@nvidia.com>
Reviewed-by: Mark Zhang <markzhang@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/counters.c |  8 +++++++-
 drivers/infiniband/core/nldev.c    | 11 ++++++++++-
 drivers/infiniband/core/sysfs.c    |  7 ++++++-
 include/rdma/ib_verbs.h            | 12 ++++++++++++
 4 files changed, 35 insertions(+), 3 deletions(-)

Comments

Jason Gunthorpe Sept. 27, 2021, 4:53 p.m. UTC | #1 
On Wed, Sep 15, 2021 at 02:07:23AM +0300, Leon Romanovsky wrote:

> diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
> index 3f6b98a87566..67519730b1ac 100644
> +++ b/drivers/infiniband/core/nldev.c
> @@ -968,15 +968,21 @@ static int fill_stat_counter_hwcounters(struct sk_buff *msg,
>  	if (!table_attr)
>  		return -EMSGSIZE;
> @@ -601,11 +604,20 @@ static inline struct rdma_hw_stats *rdma_alloc_hw_stats_struct(
>  	if (!stats)
>  		return NULL;
>  
> +	stats->is_disabled = kcalloc(BITS_TO_LONGS(num_counters),
> +				      sizeof(long), GFP_KERNEL);
> +	if (!stats->is_disabled)
> +		goto err;
> +

Please de-inline this function and make a rdma_free_hw_stats_struct()
call to pair with it. The hw_stats_data kfree should be in there. If
you do this as a precursor patch this patch will be much smaller.

Also, the 

        stats = kzalloc(sizeof(*stats) + num_counters * sizeof(u64),
                        GFP_KERNEL);

Should be using array_size

Jason
Mark Zhang Sept. 28, 2021, 3:28 a.m. UTC | #2 
On 9/28/2021 12:53 AM, Jason Gunthorpe wrote:
> On Wed, Sep 15, 2021 at 02:07:23AM +0300, Leon Romanovsky wrote:
> 
>> diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
>> index 3f6b98a87566..67519730b1ac 100644
>> +++ b/drivers/infiniband/core/nldev.c
>> @@ -968,15 +968,21 @@ static int fill_stat_counter_hwcounters(struct sk_buff *msg,
>>   	if (!table_attr)
>>   		return -EMSGSIZE;
>> @@ -601,11 +604,20 @@ static inline struct rdma_hw_stats *rdma_alloc_hw_stats_struct(
>>   	if (!stats)
>>   		return NULL;
>>   
>> +	stats->is_disabled = kcalloc(BITS_TO_LONGS(num_counters),
>> +				      sizeof(long), GFP_KERNEL);
>> +	if (!stats->is_disabled)
>> +		goto err;
>> +
> 
> Please de-inline this function and make a rdma_free_hw_stats_struct()
> call to pair with it. The hw_stats_data kfree should be in there. If
> you do this as a precursor patch this patch will be much smaller.
> 
> Also, the
> 
>          stats = kzalloc(sizeof(*stats) + num_counters * sizeof(u64),
>                          GFP_KERNEL);
> 
> Should be using array_size
Maybe use struct_size:
   stats = kzalloc(struct_size(stats, value, num_counters), GFP_KERNEL);
Jason Gunthorpe Sept. 28, 2021, 11:51 a.m. UTC | #3 
On Tue, Sep 28, 2021 at 11:28:39AM +0800, Mark Zhang wrote:
> On 9/28/2021 12:53 AM, Jason Gunthorpe wrote:
> > On Wed, Sep 15, 2021 at 02:07:23AM +0300, Leon Romanovsky wrote:
> > 
> > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
> > > index 3f6b98a87566..67519730b1ac 100644
> > > +++ b/drivers/infiniband/core/nldev.c
> > > @@ -968,15 +968,21 @@ static int fill_stat_counter_hwcounters(struct sk_buff *msg,
> > >   	if (!table_attr)
> > >   		return -EMSGSIZE;
> > > @@ -601,11 +604,20 @@ static inline struct rdma_hw_stats *rdma_alloc_hw_stats_struct(
> > >   	if (!stats)
> > >   		return NULL;
> > > +	stats->is_disabled = kcalloc(BITS_TO_LONGS(num_counters),
> > > +				      sizeof(long), GFP_KERNEL);
> > > +	if (!stats->is_disabled)
> > > +		goto err;
> > > +
> > 
> > Please de-inline this function and make a rdma_free_hw_stats_struct()
> > call to pair with it. The hw_stats_data kfree should be in there. If
> > you do this as a precursor patch this patch will be much smaller.
> > 
> > Also, the
> > 
> >          stats = kzalloc(sizeof(*stats) + num_counters * sizeof(u64),
> >                          GFP_KERNEL);
> > 
> > Should be using array_size
> Maybe use struct_size:
>   stats = kzalloc(struct_size(stats, value, num_counters), GFP_KERNEL);

Right

Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
index df9e6c5e4ddf..a9559e33a113 100644
--- a/drivers/infiniband/core/counters.c
+++ b/drivers/infiniband/core/counters.c
@@ -165,6 +165,7 @@  static struct rdma_counter *alloc_and_bind(struct ib_device *dev, u32 port,
 	return counter;
 
 err_mode:
+	kfree(counter->stats->is_disabled);
 	kfree(counter->stats);
 err_stats:
 	rdma_restrack_put(&counter->res);
@@ -186,6 +187,7 @@  static void rdma_counter_free(struct rdma_counter *counter)
 	mutex_unlock(&port_counter->lock);
 
 	rdma_restrack_del(&counter->res);
+	kfree(counter->stats->is_disabled);
 	kfree(counter->stats);
 	kfree(counter);
 }
@@ -618,6 +620,7 @@  void rdma_counter_init(struct ib_device *dev)
 fail:
 	for (i = port; i >= rdma_start_port(dev); i--) {
 		port_counter = &dev->port_data[port].port_counter;
+		kfree(port_counter->hstats->is_disabled);
 		kfree(port_counter->hstats);
 		port_counter->hstats = NULL;
 		mutex_destroy(&port_counter->lock);
@@ -631,7 +634,10 @@  void rdma_counter_release(struct ib_device *dev)
 
 	rdma_for_each_port(dev, port) {
 		port_counter = &dev->port_data[port].port_counter;
-		kfree(port_counter->hstats);
+		if (port_counter->hstats) {
+			kfree(port_counter->hstats->is_disabled);
+			kfree(port_counter->hstats);
+		}
 		mutex_destroy(&port_counter->lock);
 	}
 }
diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
index 3f6b98a87566..67519730b1ac 100644
--- a/drivers/infiniband/core/nldev.c
+++ b/drivers/infiniband/core/nldev.c
@@ -968,15 +968,21 @@  static int fill_stat_counter_hwcounters(struct sk_buff *msg,
 	if (!table_attr)
 		return -EMSGSIZE;
 
-	for (i = 0; i < st->num_counters; i++)
+	mutex_lock(&st->lock);
+	for (i = 0; i < st->num_counters; i++) {
+		if (test_bit(i, st->is_disabled))
+			continue;
 		if (rdma_nl_stat_hwcounter_entry(msg, st->descs[i].name,
 						 st->value[i]))
 			goto err;
+	}
+	mutex_unlock(&st->lock);
 
 	nla_nest_end(msg, table_attr);
 	return 0;
 
 err:
+	mutex_unlock(&st->lock);
 	nla_nest_cancel(msg, table_attr);
 	return -EMSGSIZE;
 }
@@ -2104,6 +2110,9 @@  static int stat_get_doit_default_counter(struct sk_buff *skb,
 		goto err_stats;
 	}
 	for (i = 0; i < num_cnts; i++) {
+		if (test_bit(i, stats->is_disabled))
+			continue;
+
 		v = stats->value[i] +
 			rdma_counter_get_hwstat_value(device, port, i);
 		if (rdma_nl_stat_hwcounter_entry(msg,
diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
index c3663cfdcd52..a26bf960f7ef 100644
--- a/drivers/infiniband/core/sysfs.c
+++ b/drivers/infiniband/core/sysfs.c
@@ -754,8 +754,10 @@  static void ib_port_release(struct kobject *kobj)
 
 	for (i = 0; i != ARRAY_SIZE(port->groups); i++)
 		kfree(port->groups[i].attrs);
-	if (port->hw_stats_data)
+	if (port->hw_stats_data) {
+		kfree(port->hw_stats_data->stats->is_disabled);
 		kfree(port->hw_stats_data->stats);
+	}
 	kfree(port->hw_stats_data);
 	kfree(port);
 }
@@ -919,6 +921,7 @@  alloc_hw_stats_device(struct ib_device *ibdev)
 err_free_data:
 	kfree(data);
 err_free_stats:
+	kfree(stats->is_disabled);
 	kfree(stats);
 	return ERR_PTR(-ENOMEM);
 }
@@ -926,6 +929,7 @@  alloc_hw_stats_device(struct ib_device *ibdev)
 void ib_device_release_hw_stats(struct hw_stats_device_data *data)
 {
 	kfree(data->group.attrs);
+	kfree(data->stats->is_disabled);
 	kfree(data->stats);
 	kfree(data);
 }
@@ -1018,6 +1022,7 @@  alloc_hw_stats_port(struct ib_port *port, struct attribute_group *group)
 err_free_data:
 	kfree(data);
 err_free_stats:
+	kfree(stats->is_disabled);
 	kfree(stats);
 	return ERR_PTR(-ENOMEM);
 }
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 6e484678b1fd..f016bc0cd9de 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -566,6 +566,8 @@  struct rdma_stat_desc {
  *   their own value during their allocation routine.
  * @descs - Array of pointers to static descriptors used for the counters
  *   in directory.
+ * @is_disabled - A bitmap to indicate each counter is currently disabled
+ *   or not.
  * @num_counters - How many hardware counters there are.  If name is
  *   shorter than this number, a kernel oops will result.  Driver authors
  *   are encouraged to leave BUILD_BUG_ON(ARRAY_SIZE(@name) < num_counters)
@@ -578,6 +580,7 @@  struct rdma_hw_stats {
 	unsigned long	timestamp;
 	unsigned long	lifespan;
 	const struct rdma_stat_desc *descs;
+	unsigned long	*is_disabled;
 	int		num_counters;
 	u64		value[];
 };
@@ -601,11 +604,20 @@  static inline struct rdma_hw_stats *rdma_alloc_hw_stats_struct(
 	if (!stats)
 		return NULL;
 
+	stats->is_disabled = kcalloc(BITS_TO_LONGS(num_counters),
+				      sizeof(long), GFP_KERNEL);
+	if (!stats->is_disabled)
+		goto err;
+
 	stats->descs = descs;
 	stats->num_counters = num_counters;
 	stats->lifespan = msecs_to_jiffies(lifespan);
 
 	return stats;
+
+err:
+	kfree(stats);
+	return NULL;
 }