Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Alex Williamson]

On Wed, 24 Apr 2019 12:57:58 -0500
Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:

> On Tue, Apr 23, 2019 at 04:42:28PM -0600, Alex Williamson wrote:
> > The PCIe bandwidth notification service generates logging any time a
> > link changes speed or width to a state that is considered downgraded.
> > Unfortunately, it cannot differentiate signal integrity related link
> > changes from those intentionally initiated by an endpoint driver,
> > including drivers that may live in userspace or VMs when making use
> > of vfio-pci.  Therefore, allow the driver to have a say in whether
> > the link is indeed downgraded and worth noting in the log, or if the
> > change is perhaps intentional.
> > 
> > For vfio-pci, we don't know the intentions of the user/guest driver
> > either, but we do know that GPU drivers in guests actively manage
> > the link state and therefore trigger the bandwidth notification for
> > what appear to be entirely intentional link changes.
> > 
> > Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification
> > Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@xxxxxxxxxx/T/#u
> > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > ---
> > 
> > Changing to pci_dbg() logging is not super usable, so let's try the
> > previous idea of letting the driver handle link change events as they
> > see fit.  Ideally this might be two patches, but for easier handling,
> > folding the pci and vfio-pci bits together.  Comments?  Thanks,  
> 
> I'm a little uneasy about the bandwidth notification logging as a
> whole.  Messages in dmesg don't seem like a solid base for building
> management tools.
> 
> I assume the eventual goal would be some sort of digested notification
> along the lines of "hey mr/ms administrator, the link to device X
> unexpectedly became slower, you might want to check that out."
> 
> If I were building that, I don't think I would use dmesg.  I might
> write a daemon that polls /sys/.../current_link_{speed,width}, or
> maybe use some sort of netlink event.  Maybe it would be useful to
> have the admin designate devices of interest.
> 
> I'm hesitant about adding a .link_change() handler.  If there's
> something useful a driver could do with it, that's one thing.  But
> using it merely to suppress a message doesn't really seem worth the
> trouble, and it seems unfriendly to ask drivers to add it when they
> didn't ask for it and get no benefit from it.

So where do we go from here?  I agree that dmesg is not necessarily a
great choice for these sorts of events and if they went somewhere else,
maybe I wouldn't have the same concerns about them generating user
confusion or contributing to DoS vectors from userspace drivers.  As it
is though, we have known cases where benign events generate confusing
logging messages, which seems like a regression.  Drivers didn't ask
for a link_change handler, but nor did they ask that the link state to
their device be monitored so closely.  Maybe this not only needs some
sort of change to the logging mechanism, but also an opt-in by the
driver if they don't expect runtime link changes.  Thanks,

Alex