| Message ID | alpine.DEB.2.20.1512171649070.22782@east.gentwo.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 12/17/2015 5:50 PM, Christoph Lameter wrote: > On Thu, 17 Dec 2015, Hal Rosenstock wrote: > >> On 12/17/2015 4:28 PM, Christoph Lameter wrote: >>> So port_check_extended_counters need to return another value for this. >>> The IETF ones are the uni/mcast xxx counters? >> >> Yes > > Ok. Then this patch on top of the last one should give us all of what you > want: > > > > Subject: IB core counters: Support noietf extended counters > > Detect if we have extended counters but not IETF counters. > For that we need a special table and create a function that > returns the table address. > > Signed-off-by: Christoph Lameter <cl@linux.com> > > Index: linux/drivers/infiniband/core/sysfs.c > =================================================================== > --- linux.orig/drivers/infiniband/core/sysfs.c > +++ linux/drivers/infiniband/core/sysfs.c > @@ -493,6 +493,26 @@ static struct attribute *pma_attrs_ext[] > NULL > }; > > +static struct attribute *pma_attrs_noietf[] = { > + &port_pma_attr_symbol_error.attr.attr, > + &port_pma_attr_link_error_recovery.attr.attr, > + &port_pma_attr_link_downed.attr.attr, > + &port_pma_attr_port_rcv_errors.attr.attr, > + &port_pma_attr_port_rcv_remote_physical_errors.attr.attr, > + &port_pma_attr_port_rcv_switch_relay_errors.attr.attr, > + &port_pma_attr_port_xmit_discards.attr.attr, > + &port_pma_attr_port_xmit_constraint_errors.attr.attr, > + &port_pma_attr_port_rcv_constraint_errors.attr.attr, > + &port_pma_attr_local_link_integrity_errors.attr.attr, > + &port_pma_attr_excessive_buffer_overrun_errors.attr.attr, > + &port_pma_attr_VL15_dropped.attr.attr, > + &port_pma_attr_ext_port_xmit_data.attr.attr, > + &port_pma_attr_ext_port_rcv_data.attr.attr, > + &port_pma_attr_ext_port_xmit_packets.attr.attr, > + &port_pma_attr_ext_port_rcv_packets.attr.attr, > + NULL > +}; > + > static struct attribute_group pma_group = { > .name = "counters", > .attrs = pma_attrs > @@ -503,6 +523,11 @@ static struct attribute_group pma_group_ > .attrs = pma_attrs_ext > }; > > +static struct attribute_group pma_group_noietf = { > + .name = "counters", > + .attrs = pma_attrs_noietf > +}; > + > static void ib_port_release(struct kobject *kobj) > { > struct ib_port *p = container_of(kobj, struct ib_port, kobj); > @@ -576,23 +601,32 @@ err: > } > > /* > - * Check if the port supports the Extended Counters. > - * Return error code of 0 for success > + * Figure out which counter table to use depending on > + * the device capabilities. > */ > -static int port_check_extended_counters(struct ib_device *dev) > +static struct attribute_group *get_counter_table(struct ib_device *dev) > { > int ret = 0; > struct ib_class_port_info cpi; > > ret = get_perf_mad(dev, 0, IB_PMA_CLASS_PORT_INFO, &cpi, 40, sizeof(cpi)); > > - if (ret >= 0) { > - if (!(cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH) && > - !(cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH_NOIETF)) > - ret = -ENOSYS; > + if (ret < 0) > + /* Fall back to normal counters */ > + return &pma_group; > + > + > + if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH) { > + /* We have extended counters */ > + > + if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH_NOIETF) > + /* But not the IETF ones */ > + return &pma_group_noietf; These 2 capability bits are mutually exclusive so I think it should be: if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH) { /* We have extended counters */ return &pma_group_ext; } if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH_NOIETF) /* But not the IETF ones */ return &pma_group_noietf; } return &pma_group; > + > + return &pma_group_ext; > } > > - return ret; > + return &pma_group; > } > > static int add_port(struct ib_device *device, int port_num, > @@ -623,11 +657,7 @@ static int add_port(struct ib_device *de > return ret; > } > > - ret = sysfs_create_group(&p->kobj, > - port_check_extended_counters(device) ? > - &pma_group_ext : > - &pma_group); > - > + ret = sysfs_create_group(&p->kobj, get_counter_table(device)); > if (ret) > goto err_put; > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 17 Dec 2015, Hal Rosenstock wrote: > > + if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH) { > > + /* We have extended counters */ > > + > > + if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH_NOIETF) > > + /* But not the IETF ones */ > > + return &pma_group_noietf; > > These 2 capability bits are mutually exclusive so I think it should be: > > if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH) { > /* We have extended counters */ > return &pma_group_ext; > } > > if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH_NOIETF) > /* But not the IETF ones */ > return &pma_group_noietf; This case would then use the 64 bit counters despite of the IB_PMA_CLASS_CAP_EXT_WIDTH not being set. > } > > return &pma_group; > > > + The tables contain all the counters each. So we would need another table of counters that has the ietf counters but not the 64 bit extended ones? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/18/2015 8:39 AM, Christoph Lameter wrote: > On Thu, 17 Dec 2015, Hal Rosenstock wrote: > >>> + if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH) { >>> + /* We have extended counters */ >>> + >>> + if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH_NOIETF) >>> + /* But not the IETF ones */ >>> + return &pma_group_noietf; >> >> These 2 capability bits are mutually exclusive so I think it should be: >> >> if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH) { >> /* We have extended counters */ >> return &pma_group_ext; >> } >> >> if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH_NOIETF) >> /* But not the IETF ones */ >> return &pma_group_noietf; > > This case would then use the 64 bit counters despite of the > IB_PMA_CLASS_CAP_EXT_WIDTH not being set. Yes, IB_PMA_CLASS_CAP_EXT_WIDTH means all extended counters including IETF ones whereas IB_PMA_CLASS_CAP_EXT_WIDTH_NOIETF means extended counters without IETF ones ([uni multi]cast [rcv xmit] pkts). > >> } >> >> return &pma_group; >> >>> + > > The tables contain all the counters each. So we would need another table > of counters that has the ietf counters but not the 64 bit extended ones? I'm not following what you mean. I thought pma_group_noietf and pma_group_ext take care of the 2 different options for extended counters (with the error counters needed from the mandatory PortCounters) and pma_group is when neither of the extended counter capabilities is indicated. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux/drivers/infiniband/core/sysfs.c =================================================================== --- linux.orig/drivers/infiniband/core/sysfs.c +++ linux/drivers/infiniband/core/sysfs.c @@ -493,6 +493,26 @@ static struct attribute *pma_attrs_ext[] NULL }; +static struct attribute *pma_attrs_noietf[] = { + &port_pma_attr_symbol_error.attr.attr, + &port_pma_attr_link_error_recovery.attr.attr, + &port_pma_attr_link_downed.attr.attr, + &port_pma_attr_port_rcv_errors.attr.attr, + &port_pma_attr_port_rcv_remote_physical_errors.attr.attr, + &port_pma_attr_port_rcv_switch_relay_errors.attr.attr, + &port_pma_attr_port_xmit_discards.attr.attr, + &port_pma_attr_port_xmit_constraint_errors.attr.attr, + &port_pma_attr_port_rcv_constraint_errors.attr.attr, + &port_pma_attr_local_link_integrity_errors.attr.attr, + &port_pma_attr_excessive_buffer_overrun_errors.attr.attr, + &port_pma_attr_VL15_dropped.attr.attr, + &port_pma_attr_ext_port_xmit_data.attr.attr, + &port_pma_attr_ext_port_rcv_data.attr.attr, + &port_pma_attr_ext_port_xmit_packets.attr.attr, + &port_pma_attr_ext_port_rcv_packets.attr.attr, + NULL +}; + static struct attribute_group pma_group = { .name = "counters", .attrs = pma_attrs @@ -503,6 +523,11 @@ static struct attribute_group pma_group_ .attrs = pma_attrs_ext }; +static struct attribute_group pma_group_noietf = { + .name = "counters", + .attrs = pma_attrs_noietf +}; + static void ib_port_release(struct kobject *kobj) { struct ib_port *p = container_of(kobj, struct ib_port, kobj); @@ -576,23 +601,32 @@ err: } /* - * Check if the port supports the Extended Counters. - * Return error code of 0 for success + * Figure out which counter table to use depending on + * the device capabilities. */ -static int port_check_extended_counters(struct ib_device *dev) +static struct attribute_group *get_counter_table(struct ib_device *dev) { int ret = 0; struct ib_class_port_info cpi; ret = get_perf_mad(dev, 0, IB_PMA_CLASS_PORT_INFO, &cpi, 40, sizeof(cpi)); - if (ret >= 0) { - if (!(cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH) && - !(cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH_NOIETF)) - ret = -ENOSYS; + if (ret < 0) + /* Fall back to normal counters */ + return &pma_group; + + + if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH) { + /* We have extended counters */ + + if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH_NOIETF) + /* But not the IETF ones */ + return &pma_group_noietf; + + return &pma_group_ext; } - return ret; + return &pma_group; } static int add_port(struct ib_device *device, int port_num, @@ -623,11 +657,7 @@ static int add_port(struct ib_device *de return ret; } - ret = sysfs_create_group(&p->kobj, - port_check_extended_counters(device) ? - &pma_group_ext : - &pma_group); - + ret = sysfs_create_group(&p->kobj, get_counter_table(device)); if (ret) goto err_put;