[openib-general] Re: RFC on SDP checkin

[Libor Michalek]

On Thu, Feb 17, 2005 at 04:03:59PM +0200, Michael S. Tsirkin wrote:
> Quoting r. Libor Michalek <libor at topspin.com>:
> > Possible Issues
> >   
> > [...] 
> > 
> >   - Memory locking for AIO requires a call to do_mlock() which is not a
> >     kernel exported function, the method for calling the function is not
> >     standard.
> 
> Libor, in my eyes this is the biggest issue with this gen2 sdp code.
> In short, I dont think using do_mlock is a good idea.

  I do agree that this is one of the more, if not most, problematic area
of the code, and I like your alternative suggestion below.

> Even if do_mlock *would* be exported, using this call to
> do user page locking has other limitations:
> 
> - number of locked pages is very restricted, if its not,
>   user can easily DOS the box

  I don't think the allowed locked number of pages is an issue, it is
controlled by the user and administrator, which prevents a DOS.

> - fork() is broken in many ways

  Agreed.

> - user is forbidden from doing mlock/munlock

  If either SDP or the userspace code needed to use mlock/munlock we
would really need to add reference counting to the mlocking of VMAs,
this is something that would need ot be pushed upstream. It is a
problem that the user could step on the locking that's going on
behind the scenes.

> I would like to suggest, once again, that the standard get_user_pages
> call is used, instead. It is true that virtual mappings may change,
> but it is easy to check that once the transaction is complete,
> and perform copy if this happends.
> 
> Mellanox implemented and benchmarked this on Gen1 with good results.
> 
> If you are interested, I may post the design document for how this
> works for us, or I can try and find some time to build a patch.
> However, the second option wont happend till the end of
> the month, and the patch is likely to be quite big.

  Yes, this approach was mentioned to me by both Dror and Tziporet,
at the developers workshop, and it does make sense. It does seem
that this would be a better approach then using do_mlock. I would
think that the patch would be isolated entirely to sdp_iocb.[ch]
aside from changing some function names. First we need to get FMR
support in mthca so we can get a baseline with existing code and
something to test against.


-Libor