Hi Holger, > -----Original Message----- > From: Holger Hoffstätte <holger@xxxxxxxxxxxxxxxxxxxxxx> > Sent: Saturday, September 1, 2018 11:56 AM > To: Parav Pandit <parav@xxxxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx > Subject: Re: missing counter values in sysfs > > On 09/01/18 17:44, Parav Pandit wrote: > > Rxe driver needs to implement alloc_hw_stats() and get_hw_stats() > > functions callback and those counters will be exposed in sysfs at > > /sys/class/infiniband/rxeX/ports/<Port>/counters. > They already are exposed, just with wrong content - which leads to the parsing > error. Nevertheless thanks for the hint re. the hw_counters directory; I missed > that and indeed it contains values that are updated when the rxe device has > traffic. So it really is the mapping between HW device to "IB device" that is > missing. > Sorry for the confusion. Yes, I see it. hw_counters are already implemented. Currently rxe driver choose to not implement counters and therefore N/A (no PMA) error is returned. I do not know why it is done that way. I personally think, right thing to do is, to return error code EINVAL or EOPNOTSUPP to indicate it is not supported. That makes user aware of the error. Additionally when provider driver such as rxe doesn’t implement them, counters directory shouldn't even appear. This covers both the parts of genuine error on supported HCA and not supported provider (Rxe). > > There is a library implemented in golang and used in some of the > > orchestration tool as well available at [1]. So you can directly use > > the library to integrate in the application if it is in golang. > > Now I'm confused even more. :) I really don't want to add special code to the > node collector (especially since I don't really know golang), I was wondering > whether it makes sense to catch the "N/A (no PMA)" values or do something > more sensible like maybe hiding the counters directory instead, until the hw > counters are mapped properly. IMHO no values (yet) would be better than illegal > values. Or instead of exposing wrong (string) values just set them all to 0, which > would "fix" the problem as well. > I prefer to hide it with tested patch with rxe at end of the mail thread. If there are no objections, I can generate formal patch. I have 9+ pending patches for cleanup and fixes in sysfs.c file to come in Leon's queue this week. So Once that finishes, I can send this fix later this month. > > Once rxe driver implement statistics, those will be available automatically > here. > > But it already does! See [1], so it's not the collection of values for the underlying > hw device but the connection/mapping to the "IB device" that is missing, and > that was what I was wondering about/searching for. > > thanks! > Holger > > [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driver > s/infiniband/sw/rxe/rxe_verbs.c#n1252 >From e194e28cf657dc079950d2953ec33371d5c251fa Mon Sep 17 00:00:00 2001 From: Parav Pandit <parav@xxxxxxxxxxxx> Date: Sun, 2 Sep 2018 18:45:20 -0500 Subject: [PATCH] RDMA/core: Do not expose unsupported counters If the provider driver doesn't support pma counters, avoid exposing its directory. If fails to read the PMA counter, return an error so that user can retry later. Fixes: 35c4cbb17811 ("IB/core: Create get_perf_mad function in sysfs.c") Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx> --- drivers/infiniband/core/sysfs.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c index c23b610..1784aec 100644 --- a/drivers/infiniband/core/sysfs.c +++ b/drivers/infiniband/core/sysfs.c @@ -512,7 +512,7 @@ static ssize_t show_pma_counter(struct ib_port *p, struct port_attribute *attr, ret = get_perf_mad(p->ibdev, p->port_num, tab_attr->attr_id, &data, 40 + offset / 8, sizeof(data)); if (ret < 0) - return sprintf(buf, "N/A (no PMA)\n"); + return ret; switch (width) { case 4: @@ -1046,10 +1046,12 @@ static int add_port(struct ib_device *device, int port_num, goto err_put; } - p->pma_table = get_counter_table(device, port_num); - ret = sysfs_create_group(&p->kobj, p->pma_table); - if (ret) - goto err_put_gid_attrs; + if (device->process_mad) { + p->pma_table = get_counter_table(device, port_num); + ret = sysfs_create_group(&p->kobj, p->pma_table); + if (ret) + goto err_put_gid_attrs; + } p->gid_group.name = "gids"; p->gid_group.attrs = alloc_group_attrs(show_port_gid, attr.gid_tbl_len); @@ -1157,7 +1159,8 @@ static int add_port(struct ib_device *device, int port_num, p->gid_group.attrs = NULL; err_remove_pma: - sysfs_remove_group(&p->kobj, p->pma_table); + if (device->process_mad) + sysfs_remove_group(&p->kobj, p->pma_table); err_put_gid_attrs: kobject_put(&p->gid_attr_group->kobj); @@ -1273,7 +1276,9 @@ static void free_port_list_attributes(struct ib_device *device) kfree(port->hw_stats); free_hsag(&port->kobj, port->hw_stats_ag); } - sysfs_remove_group(p, port->pma_table); + + if (port->pma_table) + sysfs_remove_group(p, port->pma_table); sysfs_remove_group(p, &port->pkey_group); sysfs_remove_group(p, &port->gid_group); sysfs_remove_group(&port->gid_attr_group->kobj, -- 1.8.3.1
