Re: [PULL REQUEST] Please pull rdma.git

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5220 bytes --]

On 03/23/2016 06:37 PM, Or Gerlitz wrote:
> On Wed, Mar 23, 2016 at 3:46 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 3/22/2016 6:23 PM, Or Gerlitz wrote:
>>> On Tue, Mar 22, 2016 at 10:50 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
>>>> Mitko Haralanov (41):
>>> [...]
>>>>       IB/hfi1: Re-factor MMU notification code
>>>>       IB/hfi1: Allow MMU function execution in IRQ context
>>>>       IB/hfi1: Prevent NULL pointer dereference
>>>>       IB/hfi1: Allow remove MMU callbacks to free nodes
>>>>       IB/hfi1: Remove the use of add/remove RB function pointers
>>>>       IB/hfi1: Notify remove MMU/RB callback of calling context
>>>>       IB/hfi1: Use interval RB trees
>>>>       IB/hfi1: Add MMU tracing
>>>>       IB/hfi1: Remove compare callback
>>>>       IB/hfi1: Add filter callback
>>>>       IB/hfi1: Adjust last address values for intervals
>>>>       IB/hfi1: Implement SDMA-side buffer caching
>>>>       IB/hfi1: Add pin query function
>>>>       IB/hfi1: Specify mm when releasing pages
>>>>       IB/hfi1: Switch to using the pin query function
>>>>       IB/hfi1: Add SDMA cache eviction algorithm
> 
>>> Doug, this series is under review and a reviewer (me...) have posted
>>> comments claiming that such pin down cache doesn't belong to kernel
>>> but rather to be part of a user-space library [1]
> 
>>> This comment still hold even though the cache is serving a proprietary
>>> (non verbs) user-space code b/c it claims that code X doesn't belong
>>> to the kernel and we need not load into the kernel what doesn't belong
>>> there.
> 
>>> Could you please comment why not let the discussion converge and/or
>>> hear your maintainer opinion on the matter before this is pushed up to Linus?
> 
>>> [1] http://marc.info/?t=145746462200001&r=1&w=2
> 
>> You made your arguments, which I thought were nebulous in the first
>> place ("does not belong in the kernel" is not a well defined objection,
>> it's like saying "I don't like that", you would need a more concise
>> objection to carry stronger weight).
> 
> Doug, by all means my review was not in a "you should do X, period" or
> "Y doesn't belong to the kernel, bump", take 5m and see:

[ snip ]

I read it all the first time Or.  I don't need to read it again.

>> Their response to your argument
>> was that putting it in the kernel saves multiple context switches per
>> memory region in the cache.  This was a sufficient answer IMO (context
>> switches are expensive and on 100GBit hardware, multiple context
>> switches that aren't needed is positively huge).
> 
> So it tool quiet a lot of time and effort to have Denny make these
> statements which are still not backed up by any published performance
> data.

Yes, so?  Do we have to have published performance numbers for every
patch?  Especially ones that intuitively sound correct?  Because I don't
see people putting those requirements on Mellanox, so I'm curious why
you think that level of burden should be placed on Intel?

> Any reason we can't hear you speaking in real time over a reviewing
> thread, and have a fair chance to convince you that things are
> actually the other way around vs. your position, but rather get your
> opinion in the form of blank pulling of patches?

You've already made your arguments.  I listened, I evaluated, I made my
decision, I moved forward.  I had (and still have) too much work to do
this merge cycle to belabor things just because you want to prevent
Intel from architecting their driver they way they want.  I would be
more inclined to listen if this wasn't private to their driver, but it
is, and you are trying to push in on their design with your own
requirements that bear no relation to the core IB code or your company's
drivers, and your requirement is one that Intel has already stated will
have a negative impact on their performance.

Intel has worked their ass off in the name of your review requests so
far.  The entire rdmavt code base was at your request.  And while
Mellanox originally promised to make their soft-rxe driver use the
rdmavt code base too, that appears to have been completely dropped at
this point.  So from my position, Intel lived up to their requirements
in regards to rdmavt, but Mellanox has not and does not appear to intend to.

So, no Or, when it comes to you being able to block Intel's work on
their hfi1 driver, you are now on a short leash.  If you make obviously
correct review comments, I'll listen.  If you make dubious review
comments, but third parties back them up, again I'll listen.  But if you
make a dubious comment, and Intel has valid reasoning for how things
are, then the burden of disproving Intel will fall on you.  At least for
a while.

> Should someone be Al
> V and/or Linus T to make comments over this list which will get you to
> speak up and have a conversation/discussion?

I have conversations on this list on a regular basis Or.  I just didn't
argue a point you wanted me to argue.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]