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
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
