[Greg, see the end for the bit which will interest you]
On Tue, Nov 25, 2008 at 03:02:01PM +1100, Vincent Rizza wrote:
> I've made quite a few changes again, got rid of the kobject and used
> an attribute group. I also tried your tests Matthew and didn't oops
> on either of them. I also wrote some code that open()'ed the file then
> continuously read() from the file while I rmmod'ed the driver. After
> the driver was removed the read() call was returning bad file
> descriptor errors until I modprobe'ed the driver back in. I ran that
> test for about 1 hour and the behaviour was the same each time.
Apologies for the completely unacceptable delay in getting back to this.
> +static ssize_t
> +pci_msix_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct msi_desc *entry;
> + unsigned long msi_index;
> + int ret = 0;
> +
> + ret = strict_strtoul(attr->attr.name, 10, &msi_index);
> + if (!ret) {
> + list_for_each_entry(entry, &pdev->msi_list, list) {
We're walking the list here without holding a lock. If the driver calls
msi_free_irqs() at exactly the right moment, we will oops. I would
suggest that RCU is probably the right tool for the job here -- rarely
updated data, and it's OK to get a slightly outdated copy of the data.
I'll admit the race is incredibly hard to hit, but it's there.
> + if (entry->msi_attrib.entry_nr == msi_index) {
> + ret = snprintf(buf, PAGE_SIZE, "%d\n",
> + entry->irq);
I think sprintf would be sufficient, at least until we get to 12288-bit
machines ... oh, and %u seems to be preferred over %d for printing
interrupt numbers.
> @@ -33,6 +34,10 @@ struct msi_desc {
> void __iomem *mask_base;
> struct pci_dev *dev;
>
> + struct device_attribute sysfs_entry;
> + /* filename = IRQ */
> + char msix_fname[16];
> +
> /* Last set MSI message */
> struct msi_msg msg;
> };
So a struct device_attribute is defined thus:
struct device_attribute {
struct attribute attr;
ssize_t (*show)(struct device *dev, struct device_attribute *attr,
char *buf);
ssize_t (*store)(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count);
};
and a struct attribute so:
struct attribute {
const char *name;
struct module *owner;
mode_t mode;
};
so, on 64-bit, that's 40 bytes, plus the 16 bytes for the filename,
an extra 56 bytes for msi_desc. That's basically doubling the size of
msi_desc (while I'd much rather shrink it).
So I have a counter-proposal. It's a bit of work though.
If we give sysfs_dir_operations an open and a release method, we could
construct directories on the fly. I don't think we'd want to do this
for all sysfs directories necessarily, but for special cases like this
where we have a large number of files, know there can't be duplicates,
and can construct the files in the directory very quickly, I think it
could be a big win.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
