Re: [RFC PATCH] PCI: export MSI mode using attributes, not kobjects

Bjorn Helgaas
2013-11-14T21:16:33Z

On Thu, Nov 14, 2013 at 1:17 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Thu, Nov 14, 2013 at 12:33:11PM -0700, Bjorn Helgaas wrote:
>> On Tue, Nov 5, 2013 at 11:55 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> > On Sat, Nov 2, 2013 at 9:50 AM, Greg Kroah-Hartman
>> > <gregkh@linuxfoundation.org> wrote:
>> >> On Fri, Nov 01, 2013 at 05:40:02PM -0600, Bjorn Helgaas wrote:
>> >>> On Tue, Oct 29, 2013 at 3:46 PM, Greg Kroah-Hartman
>> >>> <gregkh@linuxfoundation.org> wrote:
>> >>> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >>> >
>> >>> > The PCI MSI sysfs code is a mess with kobjects for things that don't
>> >>> > really need to be kobjects. This patch creates attributes dynamically
>> >>> > for the MSI interrupts instead of using kobjects.
>> >>> >
>> >>> > Note, this does not delete the existing sysfs MSI code, but puts the
>> >>> > attributes under a "msi_irqs_2" directory for testing / example.
>> >>> >
>> >>> > Also note, this removes a directory from the current MSI interrupt sysfs
>> >>> > code:
>> >>> >
>> >>> > old MSI kobjects:
>> >>> > pci_device
>> >>> > └── msi_irqs
>> >>> > └── 40
>> >>> > └── mode
>> >>> >
>> >>> > new MSI attributes:
>> >>> > pci_device
>> >>> > └── msi_irqs_2
>> >>> > └── 40
>> >>> >
>> >>> > As there was only one file "mode" with the kobject model, the interrupt
>> >>> > number is now a file that returns the "mode" of the interrupt (msi vs.
>> >>> > msix).
>> >>> >
>> >>> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >>> > ---
>> >>> >
>> >>> > Bjorn, I can make up a patch that rips out the existing kobject code
>> >>> > here, but I figured this patch would make things easier to follow
>> >>> > instead of having to dig through the removed logic at the same time.
>> >>> >
>> >>> > I'll clean up the error handling path for the create attribute logic as
>> >>> > well, this was just a proof of concept that this could be done.
>> >>> >
>> >>> > Do you think that anyone cares about the current mode files in sysfs to
>> >>> > move things in this manner?
>> >>>
>> >>> I like this a lot better than trying to fix all the holes in the
>> >>> current kobject code.
>> >>
>> >> Great.
>> >>
>> >>> I have no idea who, if anybody, cares about the "mode" files. I
>> >>> assume there's a way to create the "mode" files with attributes, too?
>> >>> If so, we could replicate the existing structure with one patch, and
>> >>> simplify it with a second patch, so it would be easier to revert the
>> >>> directory change while keeping the fix.
>> >>
>> >> No, we can't create a 2-level deep attribute at the moment, only one
>> >> level, like the patch does.
>> >>
>> >> Based on Neil's comments, I think we should be fine with this as-is as
>> >> no one is messing with these files directly (which implies that we could
>> >> possibly just remove them entirely to save us the overall pain...)
>> >
>> > Hmmm. https://bugzilla.redhat.com/show_bug.cgi?id=744012 suggests
>> > that irqbalance might be reading these files.
>>
>> I looked at the current irqbalance on github [1], and I *think* it
>> never reads the "mode" files. It reads the entries in the "msi_irqs"
>> directory, which you're proposing to change from directories to files,
>> but I think it only uses the names.
>>
> It doesn't read the mode file (I had intended for it to, but all the information
> irqbalance needs currently is implied by the fact that it appears in the sysfs
> tree under msi_irqs in the first place).
>
>> It looks like it should be safe at least for irqbalance to make this a
>> one-level attribute. It's possible we'll break somebody's scripts,
>> but I'm willing to try making this change because it really makes the
>> refcounting much simpler.
>>
> ACK, if you cc me on the patch that will change the sysfs directory structure,
> I'll make the corresponding changes needed to irqblanace in parallel.

I'm hoping *no* changes will be required to irqbalance. All it seems
to care about are the names of the entries inside "msi_irqs". The
names will stay the same; they'll just change from being directories
to being files.

If an irqbalance change *is* required, then I'm much more hesitant. I
don't want to force people to install a new irqbalance along with a
new kernel.

Bjorn