Re: [ewg] [PATCH v4] IB Core: RAW ETH support

Re: [ewg] [PATCH v4] IB Core: RAW ETH support

[Roland Dreier]

 > I tested it before on the Roland tree ( iboe branch ) and it fails,
 > because it writen in the way suitable for OFED. If adapt the patch to
 > the Roland tree, then appling Mellanox OFED patches will fail, because
 > it changes the same functions in the code.
 > Here is one example:
 > Look at __mlx4_ib_modify_qp at the Roland tree - there is no RAW_ETY
 > support. But in the OFED version of the same function this support is
 > present.
 > RAW_ETH patch modify this function and looking for RAW_ETY word and
 > without this RAW_ETH Mellanox patch will fail.

Don't take this too personally -- I picked a semi-random email in this
thread to reply to; this is pretty broadly targeted.

<rant>

What the hell is the thinking behind introducing IB_QPT_RAW_ETH?  You're
inserting an enum value before IB_QPT_RAW_ETY, so any old userspace
passing in IB_QPT_RAW_ETY will silently get different behavior depending
on the kernel version.  And you're creating two constands that differ in
a single letter (IB_QPT_RAW_ETY vs. IB_QPT_RAW_ETH).  How are you going
to explain that to users?  How is anyone ever going to get it right?
For that matter, what exactly does IB_QPT_RAW_ETH mean?

This all seems to be a symptom of how broken our development process
is.  Yes, unfortunately I can't spend as much time reviewing and
applying patches as I might like, and I apologize for that.  But if we
have all the RDMA developers piling up shit in their little area and
then sending it on to be merged as soon as it kind of works, without
thinking about design or maintainability and without ever doing any
review, then I'm always going to have an expanding review backlog.

And then we have OFED compounding problems -- "Oh that's a nice pile of
shit you've built there.  We better ship it to users while it's still
steaming."  How about if OFED developers take a little time to think
things through?

</rant>

In other words, can someone explain the plan for this raw QP stuff to me?

 - R.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH v4] IB Core: RAW ETH support

[Steve Wise]

Roland Dreier wrote:
>  > I tested it before on the Roland tree ( iboe branch ) and it fails,
>  > because it writen in the way suitable for OFED. If adapt the patch to
>  > the Roland tree, then appling Mellanox OFED patches will fail, because
>  > it changes the same functions in the code.
>  > Here is one example:
>  > Look at __mlx4_ib_modify_qp at the Roland tree - there is no RAW_ETY
>  > support. But in the OFED version of the same function this support is
>  > present.
>  > RAW_ETH patch modify this function and looking for RAW_ETY word and
>  > without this RAW_ETH Mellanox patch will fail.
>
> Don't take this too personally -- I picked a semi-random email in this
> thread to reply to; this is pretty broadly targeted.
>
> <rant>
>
> What the hell is the thinking behind introducing IB_QPT_RAW_ETH?  You're
> inserting an enum value before IB_QPT_RAW_ETY, so any old userspace
> passing in IB_QPT_RAW_ETY will silently get different behavior depending
> on the kernel version.  And you're creating two constands that differ in
> a single letter (IB_QPT_RAW_ETY vs. IB_QPT_RAW_ETH).  How are you going
> to explain that to users?  How is anyone ever going to get it right?
> For that matter, what exactly does IB_QPT_RAW_ETH mean?
>
> This all seems to be a symptom of how broken our development process
> is.  Yes, unfortunately I can't spend as much time reviewing and
> applying patches as I might like, and I apologize for that.  But if we
> have all the RDMA developers piling up shit in their little area and
> then sending it on to be merged as soon as it kind of works, without
> thinking about design or maintainability and without ever doing any
> review, then I'm always going to have an expanding review backlog.
>
> And then we have OFED compounding problems -- "Oh that's a nice pile of
> shit you've built there.  We better ship it to users while it's still
> steaming."  How about if OFED developers take a little time to think
> things through?
>
> </rant>
>
> In other words, can someone explain the plan for this raw QP stuff to me?
>
>  - R.
>   


It doesn't even look like this patch and the mlx4 patch were ever posted 
to linux-rdma. Only to the EWG list.

Granted our dev process may not be documented, but I always assumed the 
general idea was to get changes accepted upstream, then pull into ofed. 
OFED is just a mechanism to make top-of-tree linux work on distro 
kernels. There are some exceptions, but this stuff shouldn't be an 
exception.

We should all follow this "upstream first" process IMO.

Steve.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH v4] IB Core: RAW ETH support

[Moni Shoua]

 
> 
> 
> It doesn't even look like this patch and the mlx4 patch were ever posted
> to linux-rdma. Only to the EWG list.
> 
Not 100% correct. See thread from April 30.
Patches to core, libibverbs and NES driver were presented there.

> Granted our dev process may not be documented, but I always assumed the
> general idea was to get changes accepted upstream, then pull into ofed.
> OFED is just a mechanism to make top-of-tree linux work on distro
> kernels. There are some exceptions, but this stuff shouldn't be an
> exception.
> 
> We should all follow this "upstream first" process IMO.
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH v4] IB Core: RAW ETH support

[Steve Wise]

Moni Shoua wrote:
>  
>   
>> It doesn't even look like this patch and the mlx4 patch were ever posted
>> to linux-rdma. Only to the EWG list.
>>
>>     
> Not 100% correct. See thread from April 30.
> Patches to core, libibverbs and NES driver were presented there.
>
>   

I'll go look, but I thought the NES stuff was for RAW_ETY qp types.  Not 
RAW_ETH...

>> Granted our dev process may not be documented, but I always assumed the
>> general idea was to get changes accepted upstream, then pull into ofed.
>> OFED is just a mechanism to make top-of-tree linux work on distro
>> kernels. There are some exceptions, but this stuff shouldn't be an
>> exception.
>>
>> We should all follow this "upstream first" process IMO.
>>
>>     
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [ewg] [PATCH v4] IB Core: RAW ETH support

[Tung, Chien Tin]

> >> It doesn't even look like this patch and the mlx4 patch were ever posted
> >> to linux-rdma. Only to the EWG list.
> >>
> >>
> > Not 100% correct. See thread from April 30.
> > Patches to core, libibverbs and NES driver were presented there.
> >
> >
> 
> I'll go look, but I thought the NES stuff was for RAW_ETY qp types.  Not
> RAW_ETH...

There was a problem with the original patch series from Mirek and it didn't
show up in patchwork.  Subsequently there's been bug fixes including the defines.
I think it would be easier if the whole thing is cleaned up and resubmitted.
Resolving the define name would be great as I got very confused the first time
I saw it and I still don't understand it fully.

Chien


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH v4] IB Core: RAW ETH support

[Jason Gunthorpe]

On Wed, Jun 16, 2010 at 09:09:59AM -0500, Steve Wise wrote:

> Granted our dev process may not be documented, but I always assumed the 
> general idea was to get changes accepted upstream, then pull into ofed. 
> OFED is just a mechanism to make top-of-tree linux work on distro 
> kernels. There are some exceptions, but this stuff shouldn't be an 
> exception.

That is what many people wish for, me included, but it is not at all
what generally happens :(

In my observation the typical flow is:
 - A patch is written, it may or may not be sent to the list
 - 'business drivers' get it slammed into OFED right away
 - A patch is finally sent for proper review
 - It is not merged, there are comments..
 - Interest in doing anything is lost because it is already in
   OFED and that is all that matters, right?
 - People complain.

For instance, the iWarp thingy we were just discussing fits this
process rather well.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH v4] IB Core: RAW ETH support

[Steve Wise]

Jason Gunthorpe wrote:
> On Wed, Jun 16, 2010 at 09:09:59AM -0500, Steve Wise wrote:
>
>   
>> Granted our dev process may not be documented, but I always assumed the 
>> general idea was to get changes accepted upstream, then pull into ofed. 
>> OFED is just a mechanism to make top-of-tree linux work on distro 
>> kernels. There are some exceptions, but this stuff shouldn't be an 
>> exception.
>>     
>
> That is what many people wish for, me included, but it is not at all
> what generally happens :(
>
> In my observation the typical flow is:
>  - A patch is written, it may or may not be sent to the list
>  - 'business drivers' get it slammed into OFED right away
>  - A patch is finally sent for proper review
>  - It is not merged, there are comments..
>  - Interest in doing anything is lost because it is already in
>    OFED and that is all that matters, right?
>  - People complain.
>
> For instance, the iWarp thingy we were just discussing fits this
> process rather well.
>
>   

You're wrong.  I started that iWARP change in 2007 on LKLM.  I proposed 
a few ideas and show the pros/cons of each.  And it was NAKed 100% by 
mister miller.    It was then included in OFED as a last resort only 
because I couldn't get any help with trying to add this upstream in any 
form.  I even spent a few weeks developing a way to administor "iwarp 
only" ipaddresses, but Roland didn't like that scheme for various 
reasons.  So please don't mention that particular patch as a "bad 
process" unless you want to argue with me some more about it.

Also, the chelsio iWARP driver has 100% been upstream first, then 
ofed.   Some of us are indeed trying to do the right thing.

<steps off soap box>


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH v4] IB Core: RAW ETH support

[Jason Gunthorpe]

On Wed, Jun 16, 2010 at 12:12:06PM -0500, Steve Wise wrote:

>>> Granted our dev process may not be documented, but I always assumed 
>>> the general idea was to get changes accepted upstream, then pull into 
>>> ofed. OFED is just a mechanism to make top-of-tree linux work on 
>>> distro kernels. There are some exceptions, but this stuff shouldn't 
>>> be an exception.

>> That is what many people wish for, me included, but it is not at all
>> what generally happens :(
>>
>> In my observation the typical flow is:
>>  - A patch is written, it may or may not be sent to the list
>>  - 'business drivers' get it slammed into OFED right away
>>  - A patch is finally sent for proper review
>>  - It is not merged, there are comments..
>>  - Interest in doing anything is lost because it is already in
>>    OFED and that is all that matters, right?
>>  - People complain.
>>
>> For instance, the iWarp thingy we were just discussing fits this
>> process rather well.

> You're wrong.  I started that iWARP change in 2007 on LKLM.  I proposed  
> a few ideas and show the pros/cons of each.  And it was NAKed 100% by  
> mister miller.    It was then included in OFED as a last resort only  
> because I couldn't get any help with trying to add this upstream in any  
> form.  I even spent a few weeks developing a way to administor "iwarp  
> only" ipaddresses, but Roland didn't like that scheme for various  
> reasons.  So please don't mention that particular patch as a "bad  
> process" unless you want to argue with me some more about it.

Uhm, what you just described does fit my process outline:

 #1 - Patch written, sent to LKML. Check.
 #3 - Patch sent for proper review - in 2007. Check.
 #4 - Not merged. NAK by DM. Check.
 #2 - 'business drivers' force it into OFED - 'last resort' ie, iWarp
      cards can't be used without some fix. Check.
 #5 - Interest is lost. Yep, this was done in 2007, and it was idle
      till now. Check.
 #6 - People Complain. Hmm. Yep. Check.

Don't think I'm being critical toward only you, or singling out that
little iWarp patch. But it really isn't special, or different, or an
exception. Pick nearly any patch in OFED and someone will rush to its
defense with a 'we tried to follow the process and it failed, so we
did it anyway' argument.

I also didn't say this is the only way that RDMA development goes,
lots and lots of stuff goes into mainline first, from everyone.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH v4] IB Core: RAW ETH support

[Steve Wise]

Jason Gunthorpe wrote:
> On Wed, Jun 16, 2010 at 12:12:06PM -0500, Steve Wise wrote:
>
>   
>>>> Granted our dev process may not be documented, but I always assumed 
>>>> the general idea was to get changes accepted upstream, then pull into 
>>>> ofed. OFED is just a mechanism to make top-of-tree linux work on 
>>>> distro kernels. There are some exceptions, but this stuff shouldn't 
>>>> be an exception.
>>>>         
>
>   
>>> That is what many people wish for, me included, but it is not at all
>>> what generally happens :(
>>>
>>> In my observation the typical flow is:
>>>  - A patch is written, it may or may not be sent to the list
>>>  - 'business drivers' get it slammed into OFED right away
>>>  - A patch is finally sent for proper review
>>>  - It is not merged, there are comments..
>>>  - Interest in doing anything is lost because it is already in
>>>    OFED and that is all that matters, right?
>>>  - People complain.
>>>
>>> For instance, the iWarp thingy we were just discussing fits this
>>> process rather well.
>>>       
>
>   
>> You're wrong.  I started that iWARP change in 2007 on LKLM.  I proposed  
>> a few ideas and show the pros/cons of each.  And it was NAKed 100% by  
>> mister miller.    It was then included in OFED as a last resort only  
>> because I couldn't get any help with trying to add this upstream in any  
>> form.  I even spent a few weeks developing a way to administor "iwarp  
>> only" ipaddresses, but Roland didn't like that scheme for various  
>> reasons.  So please don't mention that particular patch as a "bad  
>> process" unless you want to argue with me some more about it.
>>     
>
> Uhm, what you just described does fit my process outline:
>
>  #1 - Patch written, sent to LKML. Check.
>  #3 - Patch sent for proper review - in 2007. Check.
>  #4 - Not merged. NAK by DM. Check.
>  #2 - 'business drivers' force it into OFED - 'last resort' ie, iWarp
>       cards can't be used without some fix. Check.
>  #5 - Interest is lost. Yep, this was done in 2007, and it was idle
>       till now. Check.
>  #6 - People Complain. Hmm. Yep. Check.
>
>   

Note the ordering is different.  IE I tried very hard to get the "right" 
solution designed and agreed-upon upstream.  But failed.  That's my 
bad.    I did, however help with the iWARP core code including neighbour 
update net events which did go in upstream before ofed.

> Don't think I'm being critical toward only you, or singling out that
> little iWarp patch. But it really isn't special, or different, or an
> exception. Pick nearly any patch in OFED and someone will rush to its
> defense with a 'we tried to follow the process and it failed, so we
> did it anyway' argument.
>
> I also didn't say this is the only way that RDMA development goes,
> lots and lots of stuff goes into mainline first, from everyone.
>
> Jason
>   


OFED maintainers should be more rigid, perhaps, with requiring that 
changes be accepted upstream first.  One observation is that there is no 
"OFED RDMA maintainer", aka a Roland Dreier, for the OFED code.  So each 
driver maintainer pretty much has free reign to do the right thing or 
the wrong thing...


Steve.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] IB Core: RAW ETH support

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 3830 bytes --]

On 06/16/2010 02:35 PM, Steve Wise wrote:
> Jason Gunthorpe wrote:
>> On Wed, Jun 16, 2010 at 12:12:06PM -0500, Steve Wise wrote:
>>
>>  
>>>>> Granted our dev process may not be documented, but I always assumed
>>>>> the general idea was to get changes accepted upstream, then pull
>>>>> into ofed. OFED is just a mechanism to make top-of-tree linux work
>>>>> on distro kernels. There are some exceptions, but this stuff
>>>>> shouldn't be an exception.
>>>>>         
>>
>>  
>>>> That is what many people wish for, me included, but it is not at all
>>>> what generally happens :(
>>>>
>>>> In my observation the typical flow is:
>>>>  - A patch is written, it may or may not be sent to the list
>>>>  - 'business drivers' get it slammed into OFED right away
>>>>  - A patch is finally sent for proper review
>>>>  - It is not merged, there are comments..
>>>>  - Interest in doing anything is lost because it is already in
>>>>    OFED and that is all that matters, right?
>>>>  - People complain.
>>>>
>>>> For instance, the iWarp thingy we were just discussing fits this
>>>> process rather well.
>>>>       
>>
>>  
>>> You're wrong.  I started that iWARP change in 2007 on LKLM.  I
>>> proposed  a few ideas and show the pros/cons of each.  And it was
>>> NAKed 100% by  mister miller.    It was then included in OFED as a
>>> last resort only  because I couldn't get any help with trying to add
>>> this upstream in any  form.  I even spent a few weeks developing a
>>> way to administor "iwarp  only" ipaddresses, but Roland didn't like
>>> that scheme for various  reasons.  So please don't mention that
>>> particular patch as a "bad  process" unless you want to argue with me
>>> some more about it.
>>>     
>>
>> Uhm, what you just described does fit my process outline:
>>
>>  #1 - Patch written, sent to LKML. Check.
>>  #3 - Patch sent for proper review - in 2007. Check.
>>  #4 - Not merged. NAK by DM. Check.
>>  #2 - 'business drivers' force it into OFED - 'last resort' ie, iWarp
>>       cards can't be used without some fix. Check.
>>  #5 - Interest is lost. Yep, this was done in 2007, and it was idle
>>       till now. Check.
>>  #6 - People Complain. Hmm. Yep. Check.
>>
>>   
> 
> Note the ordering is different.  IE I tried very hard to get the "right"
> solution designed and agreed-upon upstream.  But failed.  That's my
> bad.    I did, however help with the iWARP core code including neighbour
> update net events which did go in upstream before ofed.
> 
>> Don't think I'm being critical toward only you, or singling out that
>> little iWarp patch. But it really isn't special, or different, or an
>> exception. Pick nearly any patch in OFED and someone will rush to its
>> defense with a 'we tried to follow the process and it failed, so we
>> did it anyway' argument.
>>
>> I also didn't say this is the only way that RDMA development goes,
>> lots and lots of stuff goes into mainline first, from everyone.
>>
>> Jason
>>   
> 
> 
> OFED maintainers should be more rigid, perhaps, with requiring that
> changes be accepted upstream first.  One observation is that there is no
> "OFED RDMA maintainer", aka a Roland Dreier, for the OFED code.  So each
> driver maintainer pretty much has free reign to do the right thing or
> the wrong thing...

Yep, no doubt that has an impact on things.  It's for this very reason
that our next operating system is not following OFED but instead is
using upstream as its basis.  That will be true from now on with our
products.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: CFBFF194
	      http://people.redhat.com/dledford

Infiniband specific RPMs available at
	      http://people.redhat.com/dledford/Infiniband


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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
ewg mailing list
ewg-ZwoEplunGu1OwGhvXhtEPSCwEArCW2h5@public.gmane.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg

Re: [PATCH v4] IB Core: RAW ETH support

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 3433 bytes --]

On 06/16/2010 01:12 PM, Steve Wise wrote:
> Jason Gunthorpe wrote:
>> On Wed, Jun 16, 2010 at 09:09:59AM -0500, Steve Wise wrote:
>>
>>  
>>> Granted our dev process may not be documented, but I always assumed
>>> the general idea was to get changes accepted upstream, then pull into
>>> ofed. OFED is just a mechanism to make top-of-tree linux work on
>>> distro kernels. There are some exceptions, but this stuff shouldn't
>>> be an exception.
>>>     
>>
>> That is what many people wish for, me included, but it is not at all
>> what generally happens :(
>>
>> In my observation the typical flow is:
>>  - A patch is written, it may or may not be sent to the list
>>  - 'business drivers' get it slammed into OFED right away
>>  - A patch is finally sent for proper review
>>  - It is not merged, there are comments..
>>  - Interest in doing anything is lost because it is already in
>>    OFED and that is all that matters, right?
>>  - People complain.
>>
>> For instance, the iWarp thingy we were just discussing fits this
>> process rather well.
>>
>>   
> 
> You're wrong.  I started that iWARP change in 2007 on LKLM.  I proposed
> a few ideas and show the pros/cons of each.  And it was NAKed 100% by
> mister miller.    It was then included in OFED as a last resort only
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Which, of course, is the problem.  Once you have a solution besides "get
it upstream", you throw whatever you feel like into OFED instead of
whatever upstream will accept.  How long has OFED shipped the XRC stuff
now while it *still* isn't upstream?

> because I couldn't get any help with trying to add this upstream in any
> form. 

Again, OFED is part of the reason this failed.  That users had someplace
else to get "working" code besides upstream meant that you didn't have
end users putting pressure on the upstream kernel folks to accept *some*
form of solution.  So, your job was harder because there were no users
present to put pressure on mister miller or others, and then you
perpetuated the issue by caving and going back to OFED "as a last
resort".  It has become a "last resort" so often now that trying to get
things upstream first is just a sort of private joke amongst some people
I think.

> I even spent a few weeks developing a way to administor "iwarp
> only" ipaddresses, but Roland didn't like that scheme for various
> reasons.  So please don't mention that particular patch as a "bad
> process" unless you want to argue with me some more about it.
> 
> Also, the chelsio iWARP driver has 100% been upstream first, then
> ofed.   Some of us are indeed trying to do the right thing.
> 
> <steps off soap box>

OFED just needs to go away.  It's been far too abused for far too long
and it's mere existence is hindering upstream development.  I appreciate
that you attempt to do the right thing most of the time, but it really
needs to be all of the time, and you need your users right there beside
you in order to carry the weight you need in order to get solutions
designed and accepted instead of running into the brick wall you ran
into before.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: CFBFF194
	      http://people.redhat.com/dledford

Infiniband specific RPMs available at
	      http://people.redhat.com/dledford/Infiniband


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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
ewg mailing list
ewg-ZwoEplunGu1OwGhvXhtEPSCwEArCW2h5@public.gmane.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg

Re: [ewg] [PATCH v4] IB Core: RAW ETH support

[Moni Shoua]

Hi Roland,

> What the hell is the thinking behind introducing IB_QPT_RAW_ETH?  You're
> inserting an enum value before IB_QPT_RAW_ETY, so any old userspace
> passing in IB_QPT_RAW_ETY will silently get different behavior depending
> on the kernel version.  And you're creating two constands that differ in
> a single letter (IB_QPT_RAW_ETY vs. IB_QPT_RAW_ETH).  How are you going
> to explain that to users?  How is anyone ever going to get it right?
> For that matter, what exactly does IB_QPT_RAW_ETH mean?
> 
There is no qp type IBV_QPT_RAW_ETY in user space (at least not in the definitions
coming with libibverbs). In fact, libibverbs that comes with OFED defines (in verbs.h)
a qp type called IBV_QPT_RAW_ETT which equals to 7.
The patch that is under discussion here adds a new qp type IB_QPT_RAW_ETH and equals it to 7
to match the definition in user space. This indeed changes the value of IB_QPT_RAW_ETY to 8
but I don't see who can be affected since
1. No user space program that uses IB_QPT_RAW_ETY exists
2. kernel is compiled as one piece of code.

> This all seems to be a symptom of how broken our development process
> is.  Yes, unfortunately I can't spend as much time reviewing and
> applying patches as I might like, and I apologize for that.  But if we
> have all the RDMA developers piling up shit in their little area and
> then sending it on to be merged as soon as it kind of works, without
> thinking about design or maintainability and without ever doing any
> review, then I'm always going to have an expanding review backlog.
> 
I want very much to push the RAW_ETH patches to upstream and we didn't intend to skip
this stage. However,
1. It seems that upstream kernel still can't use ConnectX ETH ports. Is it right?
2. From practical (commercial) reasons it was important to us that the RAW_ETH patches 
will be in OFED. AFAIK, OFED recommends that patches will be reviewed and added first in 
upstream kernel but exceptions are allowed. Anyway, we didn't think that a review is not
necessary, we just used the OFED group for that matter.
> And then we have OFED compounding problems -- "Oh that's a nice pile of
> shit you've built there.  We better ship it to users while it's still
> steaming."  How about if OFED developers take a little time to think
> things through?
> 
> </rant>
> 
> In other words, can someone explain the plan for this raw QP stuff to me?
I hope I understand what needs to be explained but in short, we want a QP type
that takes the data in post_send() and puts it on the wire as is without adding
headers or footers. This allows the user of this kind of QP to build an entire Ethernet
packet and send it to an Ethernet switch port.

UD/RC QP with RoCEE is no good because we want to be compliant with simple ETH devices (e.g. remote
server with standard 10G NIC)
Infiniband RAW packets is no good (if I understand the spec correctly) because it adds LRH and is not
suitable for fabrics with Ethernet switch

This is why RAW_ETH was invented.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ewg] [PATCH v4] IB Core: RAW ETH support

[Roland Dreier]

 > There is no qp type IBV_QPT_RAW_ETY in user space (at least not in the definitions
 > coming with libibverbs). In fact, libibverbs that comes with OFED defines (in verbs.h)
 > a qp type called IBV_QPT_RAW_ETT which equals to 7.
 > The patch that is under discussion here adds a new qp type IB_QPT_RAW_ETH and equals it to 7
 > to match the definition in user space. This indeed changes the value of IB_QPT_RAW_ETY to 8
 > but I don't see who can be affected since
 > 1. No user space program that uses IB_QPT_RAW_ETY exists
 > 2. kernel is compiled as one piece of code.

Why renumber the _ETY enum?  Maybe it doesn't break anything serious but
why risk it?
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [ewg] [PATCH v4] IB Core: RAW ETH support

[Moni Shoua]

Thanks. I accept this remark.
We will try to address all the bothering issues regarding the patch set
of RAW QP support and resubmit them linux-rdma.
Since it won't work with Mellanox the patches will be only for Intel
(nes) driver. When time comes and Mellanox cards  can be tested for RAQ
QO feature they will benefit from the common part.



---------------------------------------
  Moni Shoua|  +972-54-5567934  


-----Original Message-----
From: Roland Dreier [mailto:rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org] 
Sent: Wednesday, June 23, 2010 8:32 PM
To: Moni Shoua
Cc: Aleksey Senin; Eli Cohen; ewg-G2znmakfqn7U1rindQTSdQ@public.gmane.org;
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Tziporet Koren; Yiftah Shahar
Subject: Re: [ewg] [PATCH v4] IB Core: RAW ETH support

 > There is no qp type IBV_QPT_RAW_ETY in user space (at least not in
the definitions  > coming with libibverbs). In fact, libibverbs that
comes with OFED defines (in verbs.h)  > a qp type called IBV_QPT_RAW_ETT
which equals to 7.
 > The patch that is under discussion here adds a new qp type
IB_QPT_RAW_ETH and equals it to 7  > to match the definition in user
space. This indeed changes the value of IB_QPT_RAW_ETY to 8  > but I
don't see who can be affected since  > 1. No user space program that
uses IB_QPT_RAW_ETY exists  > 2. kernel is compiled as one piece of
code.

Why renumber the _ETY enum?  Maybe it doesn't break anything serious but
why risk it?
--
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go
to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html