Hi,
here is a patch we use to integrate the Quadrics NICs into the Linux kernel.
The patch adds hooks to the Linux VM subsystem so that registered 'IOPROC'
devices can be informed of page table changes.
This allows the Quadrics NICs to perform user RDMAs safely, without requiring
page pinning. Looking through some of the recent IB and Ammasso discussions,
it may also prove useful to those NICs too.
This patch has been deployed in many large (1000+ CPUs) production Linux
clusters at high profile HPC sites such as LLNL and PNL. It has also been
incorporated in Linux kernel releases from HP, SGI and Bull.
I have discussed this patch with Andrew Morton and Andrea Arcangeli and they
believe now is the time to encourage further comments on whether it's
suitable to be incorporated into the mainline kernel.
Cheers,
David Addison
Quadrics Ltd
On Tue, 26 Apr 2005, David Addison wrote:
> Hi,
> here is a patch we use to integrate the Quadrics NICs into the Linux kernel.
<snip>
A few small comments below.
>
> +static inline void
> +ioproc_release(struct mm_struct *mm)
> +{
Return types on same line as function name makes grep'ing a lot
easier/nicer.
Here's the example from Documentation/CodingStyle :
int function(int x)
{
body of function
}
<snip>
> +/* ! CONFIG_IOPROC so make all hooks empty */
> +
> +#define ioproc_release(mm) do { } while (0)
> +
> +#define ioproc_sync_range(vma, start, end) do { } while (0)
> +
> +#define ioproc_invalidate_range(vma, start,end) do { } while (0)
> +
> +#define ioproc_update_range(vma, start, end) do { } while (0)
> +
> +#define ioproc_change_protection(vma, start, end, prot) do { } while (0)
> +
> +#define ioproc_sync_page(vma, addr) do { } while (0)
> +
> +#define ioproc_invalidate_page(vma, addr) do { } while (0)
> +
> +#define ioproc_update_page(vma, addr) do { } while (0)
> +
Why all these blank lines between each define? Seems like just a waste of
screen space to me.
--
Jesper Juhl
David Addison a ?crit :
> Hi,
>
> here is a patch we use to integrate the Quadrics NICs into the Linux
> kernel.
> The patch adds hooks to the Linux VM subsystem so that registered 'IOPROC'
> devices can be informed of page table changes.
> This allows the Quadrics NICs to perform user RDMAs safely, without
> requiring
> page pinning. Looking through some of the recent IB and Ammasso
> discussions,
> it may also prove useful to those NICs too.
Hi,
I worked on a similar patch to help updating a registration cache on
Myrinet. I came to the problem of deciding between registering ioproc
to the entire address space (1) or only to some VMA (2).
You're doing (1), I tried (2).
(2) avoids calling ioproc hooks for all pages that are never involved
in any communication. This might be good if the amount of pages that
are involved is not too high and if the coproc_ops cost is a little bit
high.
Do you have any numbers about this in real applications on QsNet ?
I see two drawback in (2).
First, it requires to play with the list of ioproc_ops when VMA are
merged or split. Actually, it's not that bad since the list often
contains only 1 ioproc_ops.
Secondly, you have to add the ioproc to all involved VMA at some point.
It's easy when the API asks the application to register, you just add
the ioproc_ops to the target VMA during registration. But, I guess it's
not easy with Quadrics, right ?
I see in your patch that ioproc are not inherited during fork.
How do you support fork in your driver/lib then ?
What if a COW page is given to the son and the copy to the father
while some IO are being processed ? Do you require the application to
call a specific routine after forking ?
Don't you think it might be good to add a hook in the fork code
so that ioproc are inherited or duplicated pages are invalidated
in the card ?
Regards,
Brice
On Tue, 2005-04-26 at 18:57 +0200, Jesper Juhl wrote:
> >
> > +static inline void
> > +ioproc_release(struct mm_struct *mm)
> > +{
>
> Return types on same line as function name makes grep'ing a lot
> easier/nicer.
>
> Here's the example from Documentation/CodingStyle :
>
> int function(int x)
> {
How so? I never understood the reasons. This makes it easier to grep
for everything that returns int. But you make the common case (what
file is function() defined in?) harder.
Lee
On Tue, 26 Apr 2005, Lee Revell wrote:
> On Tue, 2005-04-26 at 18:57 +0200, Jesper Juhl wrote:
> > >
> > > +static inline void
> > > +ioproc_release(struct mm_struct *mm)
> > > +{
> >
> > Return types on same line as function name makes grep'ing a lot
> > easier/nicer.
> >
> > Here's the example from Documentation/CodingStyle :
> >
> > int function(int x)
> > {
>
> How so? I never understood the reasons. This makes it easier to grep
> for everything that returns int. But you make the common case (what
> file is function() defined in?) harder.
>
I don't know what you do, but when I'm grep'ing the tree for some function
I'm often looking for its return type, having that on the same line as the
function name lets me grep for the function name and the grep output will
contain the return type and function name nicely on the same line.
--
Jesper
On Tue, 2005-04-26 at 19:20 +0200, Jesper Juhl wrote:
> I don't know what you do, but when I'm grep'ing the tree for some function
> I'm often looking for its return type, having that on the same line as the
> function name lets me grep for the function name and the grep output will
> contain the return type and function name nicely on the same line.
>
I do a lot of looking at large hunks of code I'm not familiar with and
trying to figure out how it works. It's quite handy to grep for
foo_func to see all usages, then ^foo_func to see the function. I guess
my preferred style favors people trying to grok code for the first time,
while the kernel style favors those who know it inside out.
Anyway, the coding style guidelines also state clearly that these points
are not up for debate on LKML so I'll stop now...
Lee
On Tue, 26 Apr 2005, Lee Revell wrote:
> On Tue, 2005-04-26 at 19:20 +0200, Jesper Juhl wrote:
> > I don't know what you do, but when I'm grep'ing the tree for some function
> > I'm often looking for its return type, having that on the same line as the
> > function name lets me grep for the function name and the grep output will
> > contain the return type and function name nicely on the same line.
> >
>
> I do a lot of looking at large hunks of code I'm not familiar with and
> trying to figure out how it works. It's quite handy to grep for
> foo_func to see all usages, then ^foo_func to see the function.
Have you ever looked at what "make tags" gives you?
Run make tags in the kernel source dir, then open up a source file in
vim, place the cursor over some struct name or function name and press
CTRL+] and you'll be taken to the definition, you can drill down several
levels like that, and if you want to go back up one level to where you
were you simply press CTRL+t very useful when navigating the source.
--
Jesper
On 2005-04-26T19:20:13, Jesper Juhl <[email protected]> wrote:
> I don't know what you do, but when I'm grep'ing the tree for some function
> I'm often looking for its return type, having that on the same line as the
> function name lets me grep for the function name and the grep output will
> contain the return type and function name nicely on the same line.
grep -rB1 '^function' drivers/
On Tue, Apr 26, 2005 at 01:28:31PM -0400, Lee Revell wrote:
> I do a lot of looking at large hunks of code I'm not familiar with and
> trying to figure out how it works. It's quite handy to grep for
I'd suggest cscope...
John
--
John W. Linville
[email protected]
On Tue, 2005-04-26 at 16:14 -0400, John W. Linville wrote:
> On Tue, Apr 26, 2005 at 01:28:31PM -0400, Lee Revell wrote:
>
> > I do a lot of looking at large hunks of code I'm not familiar with and
> > trying to figure out how it works. It's quite handy to grep for
>
> I'd suggest cscope...
Thanks. But now I feel bad hijacking the OP's thread.
Any comments on the patch? ;-)
Lee
Brice Goglin wrote:
> David Addison a ?crit :
>
>> Hi,
>>
>> here is a patch we use to integrate the Quadrics NICs into the Linux
>> kernel.
>> The patch adds hooks to the Linux VM subsystem so that registered
>> 'IOPROC'
>> devices can be informed of page table changes.
>> This allows the Quadrics NICs to perform user RDMAs safely, without
>> requiring
>> page pinning. Looking through some of the recent IB and Ammasso
>> discussions,
>> it may also prove useful to those NICs too.
>
>
> Hi,
>
> I worked on a similar patch to help updating a registration cache on
> Myrinet. I came to the problem of deciding between registering ioproc
> to the entire address space (1) or only to some VMA (2).
> You're doing (1), I tried (2).
>
> (2) avoids calling ioproc hooks for all pages that are never involved
> in any communication. This might be good if the amount of pages that
> are involved is not too high and if the coproc_ops cost is a little bit
> high.
> Do you have any numbers about this in real applications on QsNet ?
>
We have always taken approach (1) as it seems to be the simplest method
and offers the model where the whole user process space can be made available
for RDMA operations.
Admittedly, the update calls for pages which are not going to be used for
RDMA operations is an overhead, but the device driver can elect not to
present update functions and instead rely on pre-registration of comms
buffers.
For invalidates the device driver will have knowledge of the registered
regions and can quickly ignore irrelevant unloads.
With HPC applications in general we find the memory image is pretty static
over the life of the job and hence most of the costs are taken as the
pages are created during job startup and initialisation.
> I see two drawback in (2).
> First, it requires to play with the list of ioproc_ops when VMA are
> merged or split. Actually, it's not that bad since the list often
> contains only 1 ioproc_ops.
> Secondly, you have to add the ioproc to all involved VMA at some point.
> It's easy when the API asks the application to register, you just add
> the ioproc_ops to the target VMA during registration. But, I guess it's
> not easy with Quadrics, right ?
>
In the past we have allowed dynamic page faulting of all application exposed
memory via RDMA operations.
However, our newer libraries do now implement a registration cache so we
can pre-load translations to our NIC MMU (or pin, if kernel invalidate hooks
are not available).
However, I still prefer model (1) as it allows both implementations and
appears to be much simpler in terms of the linux kernel changes required.
>
> I see in your patch that ioproc are not inherited during fork.
> How do you support fork in your driver/lib then ?
> What if a COW page is given to the son and the copy to the father
> while some IO are being processed ? Do you require the application to
> call a specific routine after forking ?
> Don't you think it might be good to add a hook in the fork code
> so that ioproc are inherited or duplicated pages are invalidated
> in the card ?
>
Yes, on fork() our programming model is for the child to attach to the
device again. The QsNet model has a NIC MMU context for each process
so it makes sense for each process to attach and have independent
IOPROC and NIC memory management.
But you're right, there should be IOPROC hooks to ensure that the device
cannot write to COW pages after the fork. I've added a new callback for this;
ioproc_wrprotect_page() called in copy_one_pte(), and a new revised patch
is attached (Jesper: with whitespace corrections too ;-)
> Regards,
> Brice
Thanks for your comments,
Cheers
David.
Brice Goglin <[email protected]> writes:
> I see two drawback in (2).
> First, it requires to play with the list of ioproc_ops when VMA are
> merged or split. Actually, it's not that bad since the list often
> contains only 1 ioproc_ops.
I had a similar problem with the NUMA policies. With some minor hacks
you could probably reuse the policy support by making it a weird kind
of policy. That would allow to keep the fast path impact very low,
which I think is the most important part of such hardware specific
narrow purpose, useless to 99.9999% of all users hacks
(Golden rule number 1 such code: dont impact anything else)
-Andi
On Tue, Apr 26, 2005 at 04:49:01PM +0100, David Addison wrote:
> Hi,
>
> here is a patch we use to integrate the Quadrics NICs into the Linux kernel.
> The patch adds hooks to the Linux VM subsystem so that registered 'IOPROC'
> devices can be informed of page table changes.
> This allows the Quadrics NICs to perform user RDMAs safely, without
> requiring
> page pinning. Looking through some of the recent IB and Ammasso discussions,
> it may also prove useful to those NICs too.
>
I think the best thing to do is post this patch to openib-general
( http://openib.org/mailman/listinfo/openib-general )
and get a patch developed that works on amasso, IB, and Quadrics
hardware, and then come back to lkml.
> @@ -267,6 +270,11 @@
>
> unsigned long hiwater_rss; /* High-water RSS usage */
> unsigned long hiwater_vm; /* High-water virtual memory usage */
> +
> +#ifdef CONFIG_IOPROC
> + /* hooks for io devices with advanced RDMA capabilities */
> + struct ioproc_ops *ioproc_ops;
> +#endif
> };
> +int
> +ioproc_register_ops(struct mm_struct *mm, struct ioproc_ops *ip)
> +{
> + ip->next = mm->ioproc_ops;
> + mm->ioproc_ops = ip;
> +
> + return 0;
> +}
> +
> +EXPORT_SYMBOL_GPL(ioproc_register_ops);
> +
> +int
> +ioproc_unregister_ops(struct mm_struct *mm, struct ioproc_ops *ip)
> +{
> + struct ioproc_ops **tmp;
> +
> + for (tmp = &mm->ioproc_ops; *tmp && *tmp != ip; tmp= &(*tmp)->next)
> + ;
> + if (*tmp) {
> + *tmp = ip->next;
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +
> +EXPORT_SYMBOL_GPL(ioproc_unregister_ops);
You don't seem to use any synchronization mechanism to protect the
ioproc list from concurrent modifications, right ?
I understand that it might be useless as long as QsNet is the only user
of ioprocs and takes care of locking the address space somewhere in the
driver before adding/removing hooks.
But, if this patch is to be merged to the mainline, you probably need
to do something here. It's not clear how other in-kernel users
(IB, Myri, Ammasso, ...) might use ioprocs.
And actually, I think all ioproc list traversal need to be protected
as well.
A spinlock_t ioproc_lock is probably appropriate here.
I don't know whether any of the existing locks in the task_struct
might be used instead.
Regards,
Brice
On Wed, Apr 27, 2005 at 10:41:07AM +0100, David Addison wrote:
> Brice Goglin wrote:
> >I worked on a similar patch to help updating a registration cache on
> >Myrinet. I came to the problem of deciding between registering ioproc
> >to the entire address space (1) or only to some VMA (2).
> >You're doing (1), I tried (2).
>
> We have always taken approach (1) as it seems to be the simplest
> method and offers the model where the whole user process space can be
> made available for RDMA operations.
I agree that this is a nice patch for exploring the design space (and
frankly, for maintaining outside the kernel tree). I'd like to see
something like this merged. As it stands, the patch is a decent
standalone implementation of (1).
I would personally strongly prefer that whatever is merged be low-impact
and so obviously good that it would not need to be a CONFIG_ option.
(Or rather, it should be a CONFIG_ option, but one which is forced to
yes if !CONFIG_EMBEDDED.)
And of course, it needs to be general-purpose enough to satisfy all the
significant constituencies:
1. Myrinet/Quadrics (proprietary interconnects for HPC/etc)
2. Infiniband (slightly more general-purpose interconnect standard for
etc/HPC)
3. RDMA TCP
and I would add
4. people who want to add a commodity card to a general-purpose server
and be able to take advantage of direct-to-userspace transfers
without breaking the general-purposeness of their server.
I think that given a reliable framework for DMA-to-userspace, other
users will pop up. OpenGL (DRI) is one obvious example; I think there
are others.
With those (fairly lofty) goals in mind, I think the verdict is not good
for ioproc-2.6.12-rc3.patch.
It's got some style-ish issues that would have to be worked out before
it could be merged. (#ifdef in code, for one.)
It's adding a linked-list walk to a bunch of places in mm/, which is (or
at least, seems to me) pretty unacceptable (even if it's just one
cacheline miss) in the fast paths.
Did you understand Andi's suggestion about NUMA policies? (I'm not
smart enough to follow it.) Can we share code between this and the NUMA
stuff?
> static over the life of the job and hence most of the costs are taken
> as the pages are created during job startup and initialisation.
Yeah, I'm pretty skeptical about claims that "It's too much work to keep
track of all that" regarding per-proc versus per-vma, and also regarding
explicit-lock-from-commlib versus dynamic-pinning. For the people who
care (HPC), pin/unpin events are very rare (zero during normal runtime),
so the overhead is unimportant. It's more important to provide reliable
operation with minimal impact to standard mm semantics.
> However, I still prefer model (1) as it allows both implementations and
> appears to be much simpler in terms of the linux kernel changes required.
I agree that (1) looks easier to implement when you're doing it outside
the kernel (and tracking). However, if you're aiming for integration
we should figure out what the right answer is. It feels like that's
per-vma, but I freely admit I don't have any code to back that up.
> Thanks for your comments,
Thank you for stepping up to be our archery target. :)
> diff -ruN linux-2.6.12-rc3.orig/include/linux/ioproc.h linux-2.6.12-rc3.ioproc/include/linux/ioproc.h
Could you add -p to your diff invocation, please...
This patch is *exactly* what I'd want if I were looking for an obvious,
easy-to-maintain externally-maintained patch to add this capability.
(Would that I could say that for all the HPC kernel patches I've been
subjected to.)
But I think we can do better.
At least I would like to see Andi (or another NUMA mm god) and you (or
another RDMA expert) hash over the possiblity of sharing code.
-andy
Brice Goglin wrote:
>> @@ -267,6 +270,11 @@
>>
>> unsigned long hiwater_rss; /* High-water RSS usage */
>> unsigned long hiwater_vm; /* High-water virtual memory usage */
>> +
>> +#ifdef CONFIG_IOPROC
>> + /* hooks for io devices with advanced RDMA capabilities */
>> + struct ioproc_ops *ioproc_ops;
>> +#endif
>> };
>
>> +int
>> +ioproc_register_ops(struct mm_struct *mm, struct ioproc_ops *ip)
>> +{
>> + ip->next = mm->ioproc_ops;
>> + mm->ioproc_ops = ip;
>> +
>> + return 0;
>> +}
>> +
>> +EXPORT_SYMBOL_GPL(ioproc_register_ops);
>> +
>> +int
>> +ioproc_unregister_ops(struct mm_struct *mm, struct ioproc_ops *ip)
>> +{
>> + struct ioproc_ops **tmp;
>> +
>> + for (tmp = &mm->ioproc_ops; *tmp && *tmp != ip; tmp= &(*tmp)->next)
>> + ;
>> + if (*tmp) {
>> + *tmp = ip->next;
>> + return 0;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +EXPORT_SYMBOL_GPL(ioproc_unregister_ops);
>
> You don't seem to use any synchronization mechanism to protect the
> ioproc list from concurrent modifications, right ?
> I understand that it might be useless as long as QsNet is the only user
> of ioprocs and takes care of locking the address space somewhere in the
> driver before adding/removing hooks.
> But, if this patch is to be merged to the mainline, you probably need
> to do something here. It's not clear how other in-kernel users
> (IB, Myri, Ammasso, ...) might use ioprocs.
> And actually, I think all ioproc list traversal need to be protected
> as well.
>
All ioproc list traversal is protected by the mm->page_table_lock which is
held at all points where the callbacks are invoked.
[Actually there is one case where this isn't true, which I'll fix
when we refresh this patch later today]
The registration/unregister functions also need to be called holding this
spinlock, our device driver does this, but perhaps we need to document
that requirement more clearly.
Cheers
David.
On Tue, Apr 26, 2005 at 01:13:04PM -0400, Lee Revell wrote:
> On Tue, 2005-04-26 at 18:57 +0200, Jesper Juhl wrote:
> > >
> > > +static inline void
> > > +ioproc_release(struct mm_struct *mm)
> > > +{
> >
> > Return types on same line as function name makes grep'ing a lot
> > easier/nicer.
> >
> > Here's the example from Documentation/CodingStyle :
> >
> > int function(int x)
> > {
>
> How so? I never understood the reasons. This makes it easier to grep
> for everything that returns int. But you make the common case (what
> file is function() defined in?) harder.
etags/ctags end of story :)
--
/ jakob
> > +ioproc_register_ops(struct mm_struct *mm, struct ioproc_ops *ip)
> > +{
> > + ip->next = mm->ioproc_ops;
> > + mm->ioproc_ops = ip;
> > +
> > + return 0;
> > +}
> > +
Why not use a list_head along with linux standard list primitives ?
Ben.
On Tue, 2005-04-26 at 13:13 -0400, Lee Revell wrote:
> On Tue, 2005-04-26 at 18:57 +0200, Jesper Juhl wrote:
> > >
> > > +static inline void
> > > +ioproc_release(struct mm_struct *mm)
> > > +{
> >
> > Return types on same line as function name makes grep'ing a lot
> > easier/nicer.
> >
> > Here's the example from Documentation/CodingStyle :
> >
> > int function(int x)
> > {
>
> How so? I never understood the reasons. This makes it easier to grep
> for everything that returns int. But you make the common case (what
> file is function() defined in?) harder.
Not exactly. I used the 2-lines style for a while, and changed overtime
and now can't stand anything but the one line style :)
I recommend you read the mailing list archives for linus comments on
this issue btw.
Ben.
Benjamin Herrenschmidt wrote:
>>>+ioproc_register_ops(struct mm_struct *mm, struct ioproc_ops *ip)
>>>+{
>>>+ ip->next = mm->ioproc_ops;
>>>+ mm->ioproc_ops = ip;
>>>+
>>>+ return 0;
>>>+}
>>>+
>
> Why not use a list_head along with linux standard list primitives ?
>
> Ben.
>
>
The reason we didn't use the standard list primitives was that we wanted the normal
case where no ioproc ops were registered to have minimal impact and this just comes
down to mm->ioproc_ops being checked against being zero, which is slightly lighter weight
than using the list primitives.
Also entries are rarely removed from the list using the ioproc_deregister function as
in the normal case they get removed in the call to ioproc_release. Hence there is little
need for the doubly linked list.
Cheers,
David