Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

From: Vlad Zolotarov
Date: Mon Oct 05 2015 - 07:41:44 EST




On 10/05/15 13:57, Greg KH wrote:
On Mon, Oct 05, 2015 at 01:48:39PM +0300, Vlad Zolotarov wrote:

On 10/05/15 10:56, Greg KH wrote:
On Mon, Oct 05, 2015 at 10:41:39AM +0300, Vlad Zolotarov wrote:
+struct msix_info {
+ int num_irqs;
+ struct msix_entry *table;
+ struct uio_msix_irq_ctx {
+ struct eventfd_ctx *trigger; /* MSI-x vector to eventfd */
Why are you using eventfd for msi vectors? What's the reason for
needing this?
A small correction - for MSI-X vectors. There may be only one MSI vector per
PCI function and if it's used it would use the same interface as a legacy
INT#x interrupt uses at the moment.
So, for MSI-X case the reason is that there may be (in most cases there will
be) more than one interrupt vector. Thus, as I've explained in a PATCH1
thread we need a way to indicated each of them separately. eventfd seems
like a good way of doing so. If u have better ideas, pls., share.
You need to document what you are doing here, I don't see any
explaination for using eventfd at all.

And no, I don't know of any other solution as I don't know what you are
trying to do here (hint, the changelog didn't document it...)

You haven't documented how this api works at all, you are going to have
to a lot more work to justify this, as this greatly increases the
complexity of the user/kernel api in unknown ways.
I actually do documented it a bit. Pls., check PATCH3 out.
That provided no information at all about how to use the api.

If it did, you would see that your api is broken for 32/64bit kernels
and will fall over into nasty pieces the first time you try to use it
there, which means it hasn't been tested at all :(
It has been tested of course ;)
I tested it only in 64 bit environment however where both kernel and user
space applications were compiled on the same machine with the same compiler
and it could be that "int" had the same number of bytes both in kernel and
in user space application. Therefore it worked perfectly - I patched DPDK to
use the new uio_pci_generic MSI-X API to test this and I have verified that
all 3 interrupt modes work: MSI-X with SR-IOV VF device in Amazon EC2 guest
and INT#x and MSI with a PF device on bare metal server.

However I agree using uint32_t for "vec" and "fd" would be much more
correct.
I don't think file descriptors are __u32 on a 64bit arch, are they?

I think they are "int" on all platforms and as far as I know u32 should be enough to contain int on any platform.


And NEVER use the _t types in kernel code,

Never meant it - it was for a user space interface. For a kernel it's u32 of course.

the namespaces is all wrong
and it is not applicable for us, sorry.

thanks,

greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/