On Thu, Jun 21, 2018 at 11:48 AM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > [+cc Tyler for AER dmesg decoding] > > I really like this idea a lot; thanks for putting it together! > > On Wed, Jun 20, 2018 at 04:41:45PM -0700, Rajat Jain wrote: >> Add sysfs attributes to provide breakdown of the AERs seen, >> into different type of correctable or uncorrectable errors: >> >> dev_breakdown_correctable >> dev_breakdown_uncorrectable > > - Can you include a more complete sysfs path here in the commit log, > as well as a snippet of the contents? From the doc patch, I think > it is currently: > > /sys/bus/pci/devices/<dev>/aer_stats/dev_breakdown_correctable > /sys/bus/pci/devices/<dev>/aer_stats/dev_breakdown_uncorrectable > > - I'm not sure it's worth making a new subdirectory. What if you > simply added these? Its your call. We're going to be creating 6 files for aer_stats (I'll be following your suggestion below), and I think it may clutter the directory. In my next patch, I'm going to remove the sub directory, but we can add that later if you feel so. > > /sys/bus/pci/devices/<dev>/aer_correctable > /sys/bus/pci/devices/<dev>/aer_uncorrectable > > or perhaps, since you split the "total" files into > cor/nonfatal/fatal, these could match? > > /sys/bus/pci/devices/<dev>/aer_correctable > /sys/bus/pci/devices/<dev>/aer_nonfatal > /sys/bus/pci/devices/<dev>/aer_fatal This sounds like a better idea. > > I think the nonfatal/fatal distinction might be worth exposing > because some of those are configurable and the kernel handling is > significantly different. So I think it would make this more > approachable if the "remove/re-enumerate" situations that will be > obvious in dmesg logs were clearly connected with "aer_fatal" > statistics, as opposed to being connected to some subset of what's > in "aer_uncorrectable". Agree, however note that theoretically, the classification of uncorrectable errors into fatal or non fatal can be programmed / changed (by who?), so it is possible that some of the same types of errors may show up such that some instances in counted in fatal and some in non-fatal (depending on whether those bits were set while handling ERR_FATAL or ERR_NONFATAL respectively). Not that I think there is something wrong with this, just thought I will mention. > > - Possibly the totals that you currently have in dev_total_cor_errs > could even be added to the bottom of these? Not sure what direction > would be best, and as you say, there's the potential for confusion > because the individual items won't add up to the totals. If they > were in the same file, maybe that could be addressed in the label. Agree, this also sounds good. > > - Can you include the related doc update in the same patch? That way > the doc update is more likely to be backported along with the patch. Will do. > > - I was going to ask whether these should all be in a single file or > whether they should be split up so there's a separate file for each > type or error, each containing a single number. But > Documentation/filesystems/sysfs.txt says either is OK and > /sys/devices/system/node/node0/vmstat is an example of a similar > situation in an existing file, so I think what you did is perfect. Thank you, I initially thought of having a different file for each error, but then it looked like we're be having much more files - at least large enough for the number of files to overwhelm the user space. Thanks, Rajat > >> Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx> >> --- >> v5: Fix the signature >> v4: use "%llu" in place of "%llx" >> v3: Merge everything in aer.c >> >> drivers/pci/pcie/aer.c | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >> index ce0d675d7bd3..c989bb5bb6f1 100644 >> --- a/drivers/pci/pcie/aer.c >> +++ b/drivers/pci/pcie/aer.c >> @@ -587,10 +587,38 @@ aer_stats_aggregate_attr(dev_total_cor_errs); >> aer_stats_aggregate_attr(dev_total_fatal_errs); >> aer_stats_aggregate_attr(dev_total_nonfatal_errs); >> >> +#define aer_stats_breakdown_attr(field, stats_array, strings_array) \ >> + static ssize_t \ >> + field##_show(struct device *dev, struct device_attribute *attr, \ >> + char *buf) \ >> +{ \ >> + unsigned int i; \ >> + char *str = buf; \ >> + struct pci_dev *pdev = to_pci_dev(dev); \ >> + u64 *stats = pdev->aer_stats->stats_array; \ > > Nit: add a blank line here. Will do. > >> + for (i = 0; i < ARRAY_SIZE(strings_array); i++) { \ >> + if (strings_array[i]) \ >> + str += sprintf(str, "%s = 0x%llu\n", \ >> + strings_array[i], stats[i]); \ >> + else if (stats[i]) \ >> + str += sprintf(str, #stats_array "bit[%d] = 0x%llu\n",\ >> + i, stats[i]); \ > > - I like the way this uses the same text as used in dmesg > (aer_correctable_error_string[] and > aer_uncorrectable_error_string[]). > > - I think this incorrectly prints a "0x" prefix for a decimal number > (probably an artifact of your v4 change). Will do. > > - Tyler posted a patch [1] to update those dmesg strings so they match > the way lspci decodes them. I really liked that update, but we > never quite finished it. If we're going to do that, it would be > nice to do it first, so we don't publish new sysfs files, then > immediately change the labels used in them. Sure, I guess you can push them in the right order. > > - IIRC, Tyler's patch had the nice property of changing the strings so > each error name had no spaces, which would make it a little easier > to parse this sysfs file: each line would be a single identifier > followed by a single number (I would probably remove the "=" from > the middle). Will do. > > [1] https://lkml.kernel.org/r/1518034285-3543-1-git-send-email-tbaicar@xxxxxxxxxxxxxx > >> + } \ >> + return str-buf; \ >> +} \ >> +static DEVICE_ATTR_RO(field) >> + >> +aer_stats_breakdown_attr(dev_breakdown_correctable, dev_cor_errs, >> + aer_correctable_error_string); >> +aer_stats_breakdown_attr(dev_breakdown_uncorrectable, dev_uncor_errs, >> + aer_uncorrectable_error_string); >> + >> static struct attribute *aer_stats_attrs[] __ro_after_init = { >> &dev_attr_dev_total_cor_errs.attr, >> &dev_attr_dev_total_fatal_errs.attr, >> &dev_attr_dev_total_nonfatal_errs.attr, >> + &dev_attr_dev_breakdown_correctable.attr, >> + &dev_attr_dev_breakdown_uncorrectable.attr, >> NULL >> }; >> >> -- >> 2.18.0.rc1.244.gcf134e6275-goog >>