* Re: [Linux-kernel-mentees] [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
@ 2020-01-14 18:24 ` Dennis Dalessandro
0 siblings, 0 replies; 25+ messages in thread
From: madhuparnabhowmik04 @ 2020-01-14 18:34 UTC (permalink / raw)
To: dennis.dalessandro, Jason Gunthorpe, madhuparnabhowmik04
Cc: mike.marciniszyn, paulmck, linux-rdma, linux-kernel, rcu, joel,
linux-kernel-mentees
From: Dennis Dalessandro <dennis.dalessandro@intel.com>
On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
> On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
>> On Tue, Jan 14, 2020 at 09:53:45PM +0530,
>> madhuparnabhowmik04@gmail.com wrote:
>>> From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
>>>
>>> list_for_each_entry_rcu has built-in RCU and lock checking.
>>> Pass cond argument to list_for_each_entry_rcu.
>>>
>>> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
>>> drivers/infiniband/hw/hfi1/verbs.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/infiniband/hw/hfi1/verbs.c
>>> b/drivers/infiniband/hw/hfi1/verbs.c
>>> index 089e201d7550..22f2d4fd2577 100644
>>> +++ b/drivers/infiniband/hw/hfi1/verbs.c
>>> @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
>>> hfi1_packet *packet,
>>> opa_get_lid(packet->dlid, 9B));
>>> if (!mcast)
>>> goto drop;
>>> - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
>>> + list_for_each_entry_rcu(p, &mcast->qp_list, list,
>>> lockdep_is_held(&(ibp->rvp.lock))) {
>>
>> Okay, this looks reasonable
>>
>> Mike, Dennis, is this the right lock to test?
>>
>
> I'm looking at that right now actually, I don't think this is correct.
> Wanted to talk to Mike before I send a response though.
>
> -Denny
That's definitely going to throw a ton of lock dep messages. It's not
really the right lock either. Instead what we probably need to do is
what we do in the non-multicast part of the code and take the
rcu_read_lock().
I'd say hold off on this and we'll fix it right. Same goes for the qib one.
Alright, thank you for reviewing.
The rdmavt one though looks to be OK. I'll give it a test.
Thank you,
Madhuparna
-Denny
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
@ 2020-01-14 18:24 ` Dennis Dalessandro
0 siblings, 0 replies; 25+ messages in thread
From: Dennis Dalessandro @ 2020-01-14 18:24 UTC (permalink / raw)
To: Jason Gunthorpe, madhuparnabhowmik04
Cc: mike.marciniszyn, paulmck, linux-rdma, linux-kernel, rcu, joel,
linux-kernel-mentees
On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
> On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
>> On Tue, Jan 14, 2020 at 09:53:45PM +0530,
>> madhuparnabhowmik04@gmail.com wrote:
>>> From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
>>>
>>> list_for_each_entry_rcu has built-in RCU and lock checking.
>>> Pass cond argument to list_for_each_entry_rcu.
>>>
>>> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
>>> drivers/infiniband/hw/hfi1/verbs.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/infiniband/hw/hfi1/verbs.c
>>> b/drivers/infiniband/hw/hfi1/verbs.c
>>> index 089e201d7550..22f2d4fd2577 100644
>>> +++ b/drivers/infiniband/hw/hfi1/verbs.c
>>> @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
>>> hfi1_packet *packet,
>>> opa_get_lid(packet->dlid, 9B));
>>> if (!mcast)
>>> goto drop;
>>> - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
>>> + list_for_each_entry_rcu(p, &mcast->qp_list, list,
>>> lockdep_is_held(&(ibp->rvp.lock))) {
>>
>> Okay, this looks reasonable
>>
>> Mike, Dennis, is this the right lock to test?
>>
>
> I'm looking at that right now actually, I don't think this is correct.
> Wanted to talk to Mike before I send a response though.
>
> -Denny
That's definitely going to throw a ton of lock dep messages. It's not
really the right lock either. Instead what we probably need to do is
what we do in the non-multicast part of the code and take the
rcu_read_lock().
I'd say hold off on this and we'll fix it right. Same goes for the qib one.
The rdmavt one though looks to be OK. I'll give it a test.
-Denny
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
2020-01-14 18:24 ` Dennis Dalessandro
@ 2020-01-14 19:17 ` Joel Fernandes
-1 siblings, 0 replies; 25+ messages in thread
From: Joel Fernandes @ 2020-01-14 19:17 UTC (permalink / raw)
To: madhuparnabhowmik04
Cc: dennis.dalessandro, Jason Gunthorpe, mike.marciniszyn, paulmck,
frextrite, linux-kernel-mentees, rcu, linux-rdma, linux-kernel
On Wed, Jan 15, 2020 at 12:04:58AM +0530, madhuparnabhowmik04@gmail.com wrote:
> From: Dennis Dalessandro <dennis.dalessandro@intel.com>
>
> On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
> > On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
> > > On Tue, Jan 14, 2020 at 09:53:45PM +0530,
> > > madhuparnabhowmik04@gmail.com wrote:
> > > > From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
> > > >
> > > > list_for_each_entry_rcu has built-in RCU and lock checking.
> > > > Pass cond argument to list_for_each_entry_rcu.
> > > >
> > > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
> > > > drivers/infiniband/hw/hfi1/verbs.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/hfi1/verbs.c
> > > > b/drivers/infiniband/hw/hfi1/verbs.c
> > > > index 089e201d7550..22f2d4fd2577 100644
> > > > +++ b/drivers/infiniband/hw/hfi1/verbs.c
> > > > @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
> > > > hfi1_packet *packet,
> > > > opa_get_lid(packet->dlid, 9B));
> > > > if (!mcast)
> > > > goto drop;
> > > > - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
> > > > + list_for_each_entry_rcu(p, &mcast->qp_list, list,
> > > > lockdep_is_held(&(ibp->rvp.lock))) {
> > >
> > > Okay, this looks reasonable
> > >
> > > Mike, Dennis, is this the right lock to test?
> > >
> >
> > I'm looking at that right now actually, I don't think this is correct.
> > Wanted to talk to Mike before I send a response though.
> >
> > -Denny
>
> That's definitely going to throw a ton of lock dep messages. It's not really
> the right lock either. Instead what we probably need to do is what we do in
> the non-multicast part of the code and take the rcu_read_lock().
>
> I'd say hold off on this and we'll fix it right. Same goes for the qib one.
>
> Alright, thank you for reviewing.
>
> The rdmavt one though looks to be OK. I'll give it a test.
Madhuparna, there seems to be an issue with your mail client where it is not
quoting text correctly, either there is a '>' missing or there are too many.
Can you look into it and figure what's wrong with it?
thanks,
- Joel
>
> Thank you,
> Madhuparna
>
> -Denny
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
@ 2020-01-14 19:17 ` Joel Fernandes
0 siblings, 0 replies; 25+ messages in thread
From: Joel Fernandes @ 2020-01-14 19:17 UTC (permalink / raw)
To: madhuparnabhowmik04
Cc: mike.marciniszyn, paulmck, linux-rdma, dennis.dalessandro,
linux-kernel, rcu, Jason Gunthorpe, linux-kernel-mentees
On Wed, Jan 15, 2020 at 12:04:58AM +0530, madhuparnabhowmik04@gmail.com wrote:
> From: Dennis Dalessandro <dennis.dalessandro@intel.com>
>
> On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
> > On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
> > > On Tue, Jan 14, 2020 at 09:53:45PM +0530,
> > > madhuparnabhowmik04@gmail.com wrote:
> > > > From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
> > > >
> > > > list_for_each_entry_rcu has built-in RCU and lock checking.
> > > > Pass cond argument to list_for_each_entry_rcu.
> > > >
> > > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
> > > > drivers/infiniband/hw/hfi1/verbs.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/hfi1/verbs.c
> > > > b/drivers/infiniband/hw/hfi1/verbs.c
> > > > index 089e201d7550..22f2d4fd2577 100644
> > > > +++ b/drivers/infiniband/hw/hfi1/verbs.c
> > > > @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
> > > > hfi1_packet *packet,
> > > > opa_get_lid(packet->dlid, 9B));
> > > > if (!mcast)
> > > > goto drop;
> > > > - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
> > > > + list_for_each_entry_rcu(p, &mcast->qp_list, list,
> > > > lockdep_is_held(&(ibp->rvp.lock))) {
> > >
> > > Okay, this looks reasonable
> > >
> > > Mike, Dennis, is this the right lock to test?
> > >
> >
> > I'm looking at that right now actually, I don't think this is correct.
> > Wanted to talk to Mike before I send a response though.
> >
> > -Denny
>
> That's definitely going to throw a ton of lock dep messages. It's not really
> the right lock either. Instead what we probably need to do is what we do in
> the non-multicast part of the code and take the rcu_read_lock().
>
> I'd say hold off on this and we'll fix it right. Same goes for the qib one.
>
> Alright, thank you for reviewing.
>
> The rdmavt one though looks to be OK. I'll give it a test.
Madhuparna, there seems to be an issue with your mail client where it is not
quoting text correctly, either there is a '>' missing or there are too many.
Can you look into it and figure what's wrong with it?
thanks,
- Joel
>
> Thank you,
> Madhuparna
>
> -Denny
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
2020-01-14 18:24 ` Dennis Dalessandro
@ 2020-01-14 19:41 ` Jason Gunthorpe
-1 siblings, 0 replies; 25+ messages in thread
From: Jason Gunthorpe @ 2020-01-14 19:41 UTC (permalink / raw)
To: Dennis Dalessandro
Cc: madhuparnabhowmik04, mike.marciniszyn, paulmck, joel, frextrite,
linux-kernel-mentees, rcu, linux-rdma, linux-kernel
On Tue, Jan 14, 2020 at 01:24:00PM -0500, Dennis Dalessandro wrote:
> On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
> > On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
> > > On Tue, Jan 14, 2020 at 09:53:45PM +0530,
> > > madhuparnabhowmik04@gmail.com wrote:
> > > > From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
> > > >
> > > > list_for_each_entry_rcu has built-in RCU and lock checking.
> > > > Pass cond argument to list_for_each_entry_rcu.
> > > >
> > > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
> > > > drivers/infiniband/hw/hfi1/verbs.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/hfi1/verbs.c
> > > > b/drivers/infiniband/hw/hfi1/verbs.c
> > > > index 089e201d7550..22f2d4fd2577 100644
> > > > +++ b/drivers/infiniband/hw/hfi1/verbs.c
> > > > @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
> > > > hfi1_packet *packet,
> > > > opa_get_lid(packet->dlid, 9B));
> > > > if (!mcast)
> > > > goto drop;
> > > > - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
> > > > + list_for_each_entry_rcu(p, &mcast->qp_list, list,
> > > > lockdep_is_held(&(ibp->rvp.lock))) {
> > >
> > > Okay, this looks reasonable
> > >
> > > Mike, Dennis, is this the right lock to test?
> > >
> >
> > I'm looking at that right now actually, I don't think this is correct.
> > Wanted to talk to Mike before I send a response though.
> >
> > -Denny
>
> That's definitely going to throw a ton of lock dep messages. It's not really
> the right lock either. Instead what we probably need to do is what we do in
> the non-multicast part of the code and take the rcu_read_lock().
Uh.. why is this using the _rcu varient without holding the rcu lock?
That is quite wrong already.
Jason
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
@ 2020-01-14 19:41 ` Jason Gunthorpe
0 siblings, 0 replies; 25+ messages in thread
From: Jason Gunthorpe @ 2020-01-14 19:41 UTC (permalink / raw)
To: Dennis Dalessandro
Cc: mike.marciniszyn, paulmck, linux-rdma, linux-kernel, rcu, joel,
linux-kernel-mentees
On Tue, Jan 14, 2020 at 01:24:00PM -0500, Dennis Dalessandro wrote:
> On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
> > On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
> > > On Tue, Jan 14, 2020 at 09:53:45PM +0530,
> > > madhuparnabhowmik04@gmail.com wrote:
> > > > From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
> > > >
> > > > list_for_each_entry_rcu has built-in RCU and lock checking.
> > > > Pass cond argument to list_for_each_entry_rcu.
> > > >
> > > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
> > > > drivers/infiniband/hw/hfi1/verbs.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/hfi1/verbs.c
> > > > b/drivers/infiniband/hw/hfi1/verbs.c
> > > > index 089e201d7550..22f2d4fd2577 100644
> > > > +++ b/drivers/infiniband/hw/hfi1/verbs.c
> > > > @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
> > > > hfi1_packet *packet,
> > > > opa_get_lid(packet->dlid, 9B));
> > > > if (!mcast)
> > > > goto drop;
> > > > - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
> > > > + list_for_each_entry_rcu(p, &mcast->qp_list, list,
> > > > lockdep_is_held(&(ibp->rvp.lock))) {
> > >
> > > Okay, this looks reasonable
> > >
> > > Mike, Dennis, is this the right lock to test?
> > >
> >
> > I'm looking at that right now actually, I don't think this is correct.
> > Wanted to talk to Mike before I send a response though.
> >
> > -Denny
>
> That's definitely going to throw a ton of lock dep messages. It's not really
> the right lock either. Instead what we probably need to do is what we do in
> the non-multicast part of the code and take the rcu_read_lock().
Uh.. why is this using the _rcu varient without holding the rcu lock?
That is quite wrong already.
Jason
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
2020-01-14 19:41 ` [Linux-kernel-mentees] " Jason Gunthorpe
@ 2020-01-14 19:46 ` Dennis Dalessandro
-1 siblings, 0 replies; 25+ messages in thread
From: Dennis Dalessandro @ 2020-01-14 19:46 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: madhuparnabhowmik04, mike.marciniszyn, paulmck, joel, frextrite,
linux-kernel-mentees, rcu, linux-rdma, linux-kernel
On 1/14/2020 2:41 PM, Jason Gunthorpe wrote:
> On Tue, Jan 14, 2020 at 01:24:00PM -0500, Dennis Dalessandro wrote:
>> On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
>>> On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
>>>> On Tue, Jan 14, 2020 at 09:53:45PM +0530,
>>>> madhuparnabhowmik04@gmail.com wrote:
>>>>> From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
>>>>>
>>>>> list_for_each_entry_rcu has built-in RCU and lock checking.
>>>>> Pass cond argument to list_for_each_entry_rcu.
>>>>>
>>>>> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
>>>>> drivers/infiniband/hw/hfi1/verbs.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/infiniband/hw/hfi1/verbs.c
>>>>> b/drivers/infiniband/hw/hfi1/verbs.c
>>>>> index 089e201d7550..22f2d4fd2577 100644
>>>>> +++ b/drivers/infiniband/hw/hfi1/verbs.c
>>>>> @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
>>>>> hfi1_packet *packet,
>>>>> opa_get_lid(packet->dlid, 9B));
>>>>> if (!mcast)
>>>>> goto drop;
>>>>> - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
>>>>> + list_for_each_entry_rcu(p, &mcast->qp_list, list,
>>>>> lockdep_is_held(&(ibp->rvp.lock))) {
>>>>
>>>> Okay, this looks reasonable
>>>>
>>>> Mike, Dennis, is this the right lock to test?
>>>>
>>>
>>> I'm looking at that right now actually, I don't think this is correct.
>>> Wanted to talk to Mike before I send a response though.
>>>
>>> -Denny
>>
>> That's definitely going to throw a ton of lock dep messages. It's not really
>> the right lock either. Instead what we probably need to do is what we do in
>> the non-multicast part of the code and take the rcu_read_lock().
>
> Uh.. why is this using the _rcu varient without holding the rcu lock?
> That is quite wrong already.
>
Yep, seems like a bug to me. Patch forthcoming.
-Denny
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
@ 2020-01-14 19:46 ` Dennis Dalessandro
0 siblings, 0 replies; 25+ messages in thread
From: Dennis Dalessandro @ 2020-01-14 19:46 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: mike.marciniszyn, paulmck, linux-rdma, linux-kernel, rcu, joel,
linux-kernel-mentees
On 1/14/2020 2:41 PM, Jason Gunthorpe wrote:
> On Tue, Jan 14, 2020 at 01:24:00PM -0500, Dennis Dalessandro wrote:
>> On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
>>> On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
>>>> On Tue, Jan 14, 2020 at 09:53:45PM +0530,
>>>> madhuparnabhowmik04@gmail.com wrote:
>>>>> From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
>>>>>
>>>>> list_for_each_entry_rcu has built-in RCU and lock checking.
>>>>> Pass cond argument to list_for_each_entry_rcu.
>>>>>
>>>>> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
>>>>> drivers/infiniband/hw/hfi1/verbs.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/infiniband/hw/hfi1/verbs.c
>>>>> b/drivers/infiniband/hw/hfi1/verbs.c
>>>>> index 089e201d7550..22f2d4fd2577 100644
>>>>> +++ b/drivers/infiniband/hw/hfi1/verbs.c
>>>>> @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
>>>>> hfi1_packet *packet,
>>>>> opa_get_lid(packet->dlid, 9B));
>>>>> if (!mcast)
>>>>> goto drop;
>>>>> - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
>>>>> + list_for_each_entry_rcu(p, &mcast->qp_list, list,
>>>>> lockdep_is_held(&(ibp->rvp.lock))) {
>>>>
>>>> Okay, this looks reasonable
>>>>
>>>> Mike, Dennis, is this the right lock to test?
>>>>
>>>
>>> I'm looking at that right now actually, I don't think this is correct.
>>> Wanted to talk to Mike before I send a response though.
>>>
>>> -Denny
>>
>> That's definitely going to throw a ton of lock dep messages. It's not really
>> the right lock either. Instead what we probably need to do is what we do in
>> the non-multicast part of the code and take the rcu_read_lock().
>
> Uh.. why is this using the _rcu varient without holding the rcu lock?
> That is quite wrong already.
>
Yep, seems like a bug to me. Patch forthcoming.
-Denny
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
2020-01-14 18:24 ` Dennis Dalessandro
` (3 preceding siblings ...)
(?)
@ 2020-02-14 15:43 ` Madhuparna Bhowmik
2020-02-14 17:24 ` [Linux-kernel-mentees] " Dennis Dalessandro
-1 siblings, 1 reply; 25+ messages in thread
From: Madhuparna Bhowmik @ 2020-02-14 15:43 UTC (permalink / raw)
To: dennis.dalessandro, Jason Gunthorpe, madhuparna bhowmik
Cc: mike.marciniszyn, Paul E. McKenney, linux-rdma, linux-kernel,
rcu, Joel Fernandes, linux-kernel-mentees
[-- Attachment #1.1: Type: text/plain, Size: 2096 bytes --]
On Wed, Jan 15, 2020 at 12:05 AM <madhuparnabhowmik04@gmail.com> wrote:
> From: Dennis Dalessandro <dennis.dalessandro@intel.com>
>
> On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
> > On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
> >> On Tue, Jan 14, 2020 at 09:53:45PM +0530,
> >> madhuparnabhowmik04@gmail.com wrote:
> >>> From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
> >>>
> >>> list_for_each_entry_rcu has built-in RCU and lock checking.
> >>> Pass cond argument to list_for_each_entry_rcu.
> >>>
> >>> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
> >>> drivers/infiniband/hw/hfi1/verbs.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/infiniband/hw/hfi1/verbs.c
> >>> b/drivers/infiniband/hw/hfi1/verbs.c
> >>> index 089e201d7550..22f2d4fd2577 100644
> >>> +++ b/drivers/infiniband/hw/hfi1/verbs.c
> >>> @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
> >>> hfi1_packet *packet,
> >>> opa_get_lid(packet->dlid, 9B));
> >>> if (!mcast)
> >>> goto drop;
> >>> - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
> >>> + list_for_each_entry_rcu(p, &mcast->qp_list, list,
> >>> lockdep_is_held(&(ibp->rvp.lock))) {
> >>
> >> Okay, this looks reasonable
> >>
> >> Mike, Dennis, is this the right lock to test?
> >>
> >
> > I'm looking at that right now actually, I don't think this is correct.
> > Wanted to talk to Mike before I send a response though.
> >
> > -Denny
>
> That's definitely going to throw a ton of lock dep messages. It's not
> really the right lock either. Instead what we probably need to do is
> what we do in the non-multicast part of the code and take the
> rcu_read_lock().
>
> I'd say hold off on this and we'll fix it right. Same goes for the qib one.
>
> Alright, thank you for reviewing.
>
> The rdmavt one though looks to be OK. I'll give it a test.
>
> Hi,
I just wanted to follow up on this.
Any updates?
Also, is the bug fixed now?
Thank you,
Madhuparna
> Thank you,
> Madhuparna
>
> -Denny
>
[-- Attachment #1.2: Type: text/html, Size: 3465 bytes --]
[-- Attachment #2: Type: text/plain, Size: 201 bytes --]
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
2020-02-14 15:43 ` Madhuparna Bhowmik
@ 2020-02-14 17:24 ` Dennis Dalessandro
0 siblings, 0 replies; 25+ messages in thread
From: Dennis Dalessandro @ 2020-02-14 17:24 UTC (permalink / raw)
To: Madhuparna Bhowmik, Jason Gunthorpe
Cc: mike.marciniszyn, Paul E. McKenney, Joel Fernandes, Amol Grover,
linux-kernel-mentees, rcu, linux-rdma, linux-kernel
On 2/14/2020 10:43 AM, Madhuparna Bhowmik wrote:
>
>
> On Wed, Jan 15, 2020 at 12:05 AM <madhuparnabhowmik04@gmail.com
> <mailto:madhuparnabhowmik04@gmail.com>> wrote:
>
> From: Dennis Dalessandro <dennis.dalessandro@intel.com
> <mailto:dennis.dalessandro@intel.com>>
>
> On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
> > On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
> >> On Tue, Jan 14, 2020 at 09:53:45PM +0530,
> >> madhuparnabhowmik04@gmail.com
> <mailto:madhuparnabhowmik04@gmail.com> wrote:
> >>> From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com
> <mailto:madhuparnabhowmik04@gmail.com>>
> >>>
> >>> list_for_each_entry_rcu has built-in RCU and lock checking.
> >>> Pass cond argument to list_for_each_entry_rcu.
> >>>
> >>> Signed-off-by: Madhuparna Bhowmik
> <madhuparnabhowmik04@gmail.com <mailto:madhuparnabhowmik04@gmail.com>>
> >>> drivers/infiniband/hw/hfi1/verbs.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/infiniband/hw/hfi1/verbs.c
> >>> b/drivers/infiniband/hw/hfi1/verbs.c
> >>> index 089e201d7550..22f2d4fd2577 100644
> >>> +++ b/drivers/infiniband/hw/hfi1/verbs.c
> >>> @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
> >>> hfi1_packet *packet,
> >>> opa_get_lid(packet->dlid, 9B));
> >>> if (!mcast)
> >>> goto drop;
> >>> - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
> >>> + list_for_each_entry_rcu(p, &mcast->qp_list, list,
> >>> lockdep_is_held(&(ibp->rvp.lock))) {
> >>
> >> Okay, this looks reasonable
> >>
> >> Mike, Dennis, is this the right lock to test?
> >>
> >
> > I'm looking at that right now actually, I don't think this is
> correct.
> > Wanted to talk to Mike before I send a response though.
> >
> > -Denny
>
> That's definitely going to throw a ton of lock dep messages. It's not
> really the right lock either. Instead what we probably need to do is
> what we do in the non-multicast part of the code and take the
> rcu_read_lock().
>
> I'd say hold off on this and we'll fix it right. Same goes for the
> qib one.
>
> Alright, thank you for reviewing.
>
> The rdmavt one though looks to be OK. I'll give it a test.
>
> Hi,
> I just wanted to follow up on this.
> Any updates?
> Also, is the bug fixed now?
>
> Thank you,
> Madhuparna
>
> Thank you,
> Madhuparna
>
> -Denny
>
I've got a patch going through internal discussion and testing for
adding rcu read locking.
The RDMAVT patch, I was OK with going in, I guess I just mentioned that
in a reply rather than adding an RB tag. Let me go ahead and do that.
-Denny
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
@ 2020-02-14 17:24 ` Dennis Dalessandro
0 siblings, 0 replies; 25+ messages in thread
From: Dennis Dalessandro @ 2020-02-14 17:24 UTC (permalink / raw)
To: Madhuparna Bhowmik, Jason Gunthorpe
Cc: mike.marciniszyn, Paul E. McKenney, linux-rdma, linux-kernel,
rcu, Joel Fernandes, linux-kernel-mentees
On 2/14/2020 10:43 AM, Madhuparna Bhowmik wrote:
>
>
> On Wed, Jan 15, 2020 at 12:05 AM <madhuparnabhowmik04@gmail.com
> <mailto:madhuparnabhowmik04@gmail.com>> wrote:
>
> From: Dennis Dalessandro <dennis.dalessandro@intel.com
> <mailto:dennis.dalessandro@intel.com>>
>
> On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
> > On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
> >> On Tue, Jan 14, 2020 at 09:53:45PM +0530,
> >> madhuparnabhowmik04@gmail.com
> <mailto:madhuparnabhowmik04@gmail.com> wrote:
> >>> From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com
> <mailto:madhuparnabhowmik04@gmail.com>>
> >>>
> >>> list_for_each_entry_rcu has built-in RCU and lock checking.
> >>> Pass cond argument to list_for_each_entry_rcu.
> >>>
> >>> Signed-off-by: Madhuparna Bhowmik
> <madhuparnabhowmik04@gmail.com <mailto:madhuparnabhowmik04@gmail.com>>
> >>> drivers/infiniband/hw/hfi1/verbs.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/infiniband/hw/hfi1/verbs.c
> >>> b/drivers/infiniband/hw/hfi1/verbs.c
> >>> index 089e201d7550..22f2d4fd2577 100644
> >>> +++ b/drivers/infiniband/hw/hfi1/verbs.c
> >>> @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
> >>> hfi1_packet *packet,
> >>> opa_get_lid(packet->dlid, 9B));
> >>> if (!mcast)
> >>> goto drop;
> >>> - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
> >>> + list_for_each_entry_rcu(p, &mcast->qp_list, list,
> >>> lockdep_is_held(&(ibp->rvp.lock))) {
> >>
> >> Okay, this looks reasonable
> >>
> >> Mike, Dennis, is this the right lock to test?
> >>
> >
> > I'm looking at that right now actually, I don't think this is
> correct.
> > Wanted to talk to Mike before I send a response though.
> >
> > -Denny
>
> That's definitely going to throw a ton of lock dep messages. It's not
> really the right lock either. Instead what we probably need to do is
> what we do in the non-multicast part of the code and take the
> rcu_read_lock().
>
> I'd say hold off on this and we'll fix it right. Same goes for the
> qib one.
>
> Alright, thank you for reviewing.
>
> The rdmavt one though looks to be OK. I'll give it a test.
>
> Hi,
> I just wanted to follow up on this.
> Any updates?
> Also, is the bug fixed now?
>
> Thank you,
> Madhuparna
>
> Thank you,
> Madhuparna
>
> -Denny
>
I've got a patch going through internal discussion and testing for
adding rcu read locking.
The RDMAVT patch, I was OK with going in, I guess I just mentioned that
in a reply rather than adding an RB tag. Let me go ahead and do that.
-Denny
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
2020-02-14 17:24 ` [Linux-kernel-mentees] " Dennis Dalessandro
(?)
@ 2020-02-14 17:32 ` Madhuparna Bhowmik
-1 siblings, 0 replies; 25+ messages in thread
From: Madhuparna Bhowmik @ 2020-02-14 17:32 UTC (permalink / raw)
To: Dennis Dalessandro
Cc: mike.marciniszyn, Paul E. McKenney, linux-rdma, linux-kernel,
rcu, Jason Gunthorpe, Joel Fernandes, linux-kernel-mentees
[-- Attachment #1.1: Type: text/plain, Size: 3188 bytes --]
On Fri, Feb 14, 2020 at 10:55 PM Dennis Dalessandro <
dennis.dalessandro@intel.com> wrote:
> On 2/14/2020 10:43 AM, Madhuparna Bhowmik wrote:
> >
> >
> > On Wed, Jan 15, 2020 at 12:05 AM <madhuparnabhowmik04@gmail.com
> > <mailto:madhuparnabhowmik04@gmail.com>> wrote:
> >
> > From: Dennis Dalessandro <dennis.dalessandro@intel.com
> > <mailto:dennis.dalessandro@intel.com>>
> >
> > On 1/14/2020 12:00 PM, Dennis Dalessandro wrote:
> > > On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
> > >> On Tue, Jan 14, 2020 at 09:53:45PM +0530,
> > >> madhuparnabhowmik04@gmail.com
> > <mailto:madhuparnabhowmik04@gmail.com> wrote:
> > >>> From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com
> > <mailto:madhuparnabhowmik04@gmail.com>>
> > >>>
> > >>> list_for_each_entry_rcu has built-in RCU and lock checking.
> > >>> Pass cond argument to list_for_each_entry_rcu.
> > >>>
> > >>> Signed-off-by: Madhuparna Bhowmik
> > <madhuparnabhowmik04@gmail.com <mailto:madhuparnabhowmik04@gmail.com
> >>
> > >>> drivers/infiniband/hw/hfi1/verbs.c | 2 +-
> > >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/infiniband/hw/hfi1/verbs.c
> > >>> b/drivers/infiniband/hw/hfi1/verbs.c
> > >>> index 089e201d7550..22f2d4fd2577 100644
> > >>> +++ b/drivers/infiniband/hw/hfi1/verbs.c
> > >>> @@ -515,7 +515,7 @@ static inline void hfi1_handle_packet(struct
> > >>> hfi1_packet *packet,
> > >>> opa_get_lid(packet->dlid, 9B));
> > >>> if (!mcast)
> > >>> goto drop;
> > >>> - list_for_each_entry_rcu(p, &mcast->qp_list, list) {
> > >>> + list_for_each_entry_rcu(p, &mcast->qp_list, list,
> > >>> lockdep_is_held(&(ibp->rvp.lock))) {
> > >>
> > >> Okay, this looks reasonable
> > >>
> > >> Mike, Dennis, is this the right lock to test?
> > >>
> > >
> > > I'm looking at that right now actually, I don't think this is
> > correct.
> > > Wanted to talk to Mike before I send a response though.
> > >
> > > -Denny
> >
> > That's definitely going to throw a ton of lock dep messages. It's not
> > really the right lock either. Instead what we probably need to do is
> > what we do in the non-multicast part of the code and take the
> > rcu_read_lock().
> >
> > I'd say hold off on this and we'll fix it right. Same goes for the
> > qib one.
> >
> > Alright, thank you for reviewing.
> >
> > The rdmavt one though looks to be OK. I'll give it a test.
> >
> > Hi,
> > I just wanted to follow up on this.
> > Any updates?
> > Also, is the bug fixed now?
> >
> > Thank you,
> > Madhuparna
> >
> > Thank you,
> > Madhuparna
> >
> > -Denny
> >
>
> I've got a patch going through internal discussion and testing for
> adding rcu read locking.
>
> The RDMAVT patch, I was OK with going in, I guess I just mentioned that
> in a reply rather than adding an RB tag. Let me go ahead and do that.
>
> Thank you very much for the update and Reviewed By.
Regards,
Madhuparna
-Denny
>
[-- Attachment #1.2: Type: text/html, Size: 5418 bytes --]
[-- Attachment #2: Type: text/plain, Size: 201 bytes --]
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 25+ messages in thread