Re: [PATCHv4 next 0/3] Limiting pci access

[Keith Busch <keith.busch@intel.com>]

On Mon, Nov 12, 2018 at 10:05:11PM -0800, Bjorn Helgaas wrote:
> On Tue, Dec 13, 2016 at 06:18:40PM -0500, Keith Busch wrote:
> > It's the MSI-x masking that's our next highest contributor. Masking
> > vectors still requires non-posted commands, and since they're not
> > going through managed API accessors like config space uses, the
> > removed flag is needed for checking before doing significant MMIO.
> 
> Sorry for digging up this ancient history, but do you remember where
> this MSI-X masking with non-posted commands happens?  The masking is
> an MMIO write (posted) and there should only be a non-posted MMIO read
> if we use the msi_set_mask_bit() path.
> 
> I'm looking at this path:
> 
>   nvme_pci_disable
>     pci_free_irq_vectors
>       pci_disable_msix
>         pci_msix_shutdown
>           if (pci_dev_is_disconnected())    # added by 0170591bb067
>             return                          # (skip hw access)
>           for_each_pci_msi_entry(...)       # <-- loop
>             __pci_msix_desc_mask_irq
>               writel                        # <-- MMIO write
>           pci_read_config_word(PCI_MSIX_FLAGS)
>           pci_write_config_word(PCI_MSIX_FLAGS)
>         free_msi_irqs
> 
> whih only does MMIO *writes*, which I think are posted and do not
> require completion and should not cause timeouts.  That's wasted
> effort, I agree, but it doesn't seem like it should be a performance
> issue.
>
> So there must be another path, probably preceding this one (since
> pci_disable_msix() cleans everything up), that masks the vectors and
> does the non-posted reads?
> 
> The only places we do MMIO reads are msix_program_entries(), which is
> done at MSI-X enable time, and msi_set_mask_bit(), which is used in
> irq_chip.irq_mask() and irq_chip.irq_unmask() methods.  But I haven't
> figured out where that irq_chip path is used in a loop.

I must have thought masking was a read-modify-write operation. It looks
like we're using a cached value so only posted commands needed.