On Wed, Jun 10, 2015 at 9:49 PM, Jason Gunthorpe
<jguntho...@obsidianresearch.com> wrote:
> On Wed, Jun 10, 2015 at 06:08:30PM +0300, Matan Barak wrote:
>> >It isn't really a cleanup because the whole gid table is new code and
>> >has latent elements for rocev2 - this is why it is so much bigger than
>> >it should be.
>>
>> I disagree. Could you please point on anything that is RoCE V2 specific?
>> The essence of RoCE V2 in the previous series was the gid_type part.
>> This is now completely removed.
>
> Sure gid_type is gone, but I didn't say roceve2 specific, I said
> latent elements. ie I'm assuming reasons for the scary locking are
> because the ripped out rocev2 code needed it? And some of the
> complexity that looks pointless now was supporting ripped out rocev2
> elements? That is not necessarily bad, but the code had better be good
> quailty and working..
>
Why do you think the locks have anything to do with roce v2?
> But then I look at the patches, and the very first locking I test out
> looks wrong. I see call_rcu/synchronize_rcu being used without a
> single call to rcu_read_lock. So this fails #2 of the RCU review
> checklist (Seriously? Why am I catching this?)
>
> I stopped reading at that point.
>
Well, that's easy to explain - write_gid could be called with one of
roce_gid_table's find API.
The find API also returns a ndev. The RCU solves the following scenario:
find is called and returns a ndev
write_gid is called and calls dev_put(ndev)
ndev is freed
find uses the ndev
By calling the find API in RCU, your ndev is protected.
> I think you've got the right basic idea for a cleanup series here. It
> is time to buckle down and execute it well. Do an internal mellanox
> kernel team review of this series. Audit and fix all the locking,
> evaluate the code growth and design. Audit to confirm there is no
> functional change that is not documented in a commit message. Tell me
> v6 is the best effort *team Mellanox* can put forward.
>
Jason, I really appreciate your review. If you have any comments, I
would like to
either fix or write you back. This series wasn't sent without being
looked at by the
internal team here.
> Jason
Matan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
