On 05/20/2016 12:53 PM, Mark Bloch wrote: > > >> -----Original Message----- >> From: Doug Ledford [mailto:dledford@xxxxxxxxxx] >> Sent: Friday, May 20, 2016 7:15 PM >> To: linux-rdma@xxxxxxxxxxxxxxx >> Cc: Christoph Lameter <cl@xxxxxxxxx>; Mark Bloch <markb@xxxxxxxxxxxx>; >> Doug Ledford <dledford@xxxxxxxxxx> >> Subject: [PATCH v4] IB/core: Make device counter infrastructure dynamic > [snip] >> +static struct rdma_hw_stats *iwch_alloc_stats(struct ib_device *ibdev, >> + u8 port_num) >> +{ >> + struct rdma_hw_stats *stats; >> + >> + /* Our driver only supports device level stats */ >> + if (port_num != 0) >> + return NULL; >> + >> + /* Guard against buggy edits of the names array */ >> + BUILD_BUG_ON(ARRAY_SIZE(names) != NR_COUNTERS); >> + >> + stats = kzalloc(sizeof(*stats) + NR_COUNTERS * sizeof(u64), >> GFP_KERNEL); >> + if (!stats) >> + return NULL; >> + stats->name = names; >> + stats->num_counters = NR_COUNTERS; >> + stats->lifespan = -1; >> + return stats; >> +} > [snip] > >> +static struct rdma_hw_stats *c4iw_alloc_stats(struct ib_device *ibdev, >> + u8 port_num) >> +{ >> + struct rdma_hw_stats *stats; >> + >> + BUILD_BUG_ON(ARRAY_SIZE(names) != NR_COUNTERS); >> + >> + if (port_num != 0) >> + return NULL; >> + >> + stats = kzalloc(sizeof(*stats) + NR_COUNTERS * sizeof(u64), >> GFP_KERNEL); >> + if (!stats) >> + return NULL; >> + stats->name = names; >> + stats->num_counters = NR_COUNTERS; >> + stats->lifespan = -1; >> + return stats; >> +} > [snip] >> +static struct rdma_hw_stats *i40iw_alloc_hw_stats(struct ib_device *ibdev, >> + u8 port_num) >> +{ >> + struct i40iw_device *iwdev = to_iwdev(ibdev); >> + struct i40iw_sc_dev *dev = &iwdev->sc_dev; >> + struct rdma_hw_stats *stats; >> + >> + BUILD_BUG_ON(ARRAY_SIZE(i40iw_hw_stat_names) != >> + (I40IW_HW_STAT_INDEX_MAX_32 + >> I40IW_HW_STAT_INDEX_MAX_64)); >> + >> + stats = kzalloc(sizeof(*stats) + >> + I40IW_HW_STAT_INDEX_MAX_32 * sizeof(u64) + >> + I40IW_HW_STAT_INDEX_MAX_64 * sizeof(u64), >> GFP_KERNEL); >> + if (!stats) >> + return NULL; >> + stats->name = i40iw_hw_stat_names; >> + stats->num_counters = I40IW_HW_STAT_INDEX_MAX_32 + >> + I40IW_HW_STAT_INDEX_MAX_64; >> + /* >> + * PFs get the default update lifespan, but VFs only update once >> + * per second >> + */ >> + if (dev->is_pf) >> + stats->lifespan = -1; >> + else >> + stats->lifespan = msecs_to_jiffies(1000); >> + return stats; >> +} > Just a little nitpick, what about creating a function that allocates the rdma_hw_stats struct? > Something like: > > static inline static struct rdma_hw_stats *allocate_hw_stats_struct(const char **name, > int num_counters, > unsigned long lifespan) > { > struct rdma_hw_stats *stats; > > stats = kzalloc(sizeof(*stats) + num_counters * sizeof(u64)); > if (!stats) > return NULL; > > stats->name = name; > stats->num_counters = num_counters; > if (lifespan == -1) > stats->lifespan = -1; > else > stats->lifespan = msecs_to_jiffies(lifespan); > > return stats; > } > > And putting it in ib_verbs.h? > I feel like that copying this piece of code across drivers (3 with this patch, adding mlx5 + mlx4 makes it 5) > makes little sense, also I don't feel the drivers should care/know about how to allocate it. > Specifically: kzalloc(sizeof(*stats) + num_counters * sizeof(u64)) seems out of place inside a driver, > It should be part of the infrastructure. Sure, I'll add that. I've got to fix the build issue too. > Other than that, it looks really good. > > Mark > -- Doug Ledford <dledford@xxxxxxxxxx> GPG KeyID: 0E572FDD
Attachment:
signature.asc
Description: OpenPGP digital signature
