From: Madhuparna Bhowmik <[email protected]>
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 <[email protected]>
---
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
--- a/drivers/infiniband/hw/hfi1/verbs.c
+++ 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))) {
packet->qp = p->qp;
if (hfi1_do_pkey_check(packet))
goto drop;
--
2.17.1
On Tue, Jan 14, 2020 at 09:53:45PM +0530, [email protected] wrote:
> From: Madhuparna Bhowmik <[email protected]>
>
> 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 <[email protected]>
> 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?
Jason
On 1/14/2020 11:57 AM, Jason Gunthorpe wrote:
> On Tue, Jan 14, 2020 at 09:53:45PM +0530, [email protected] wrote:
>> From: Madhuparna Bhowmik <[email protected]>
>>
>> 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 <[email protected]>
>> 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
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,
>> [email protected] wrote:
>>> From: Madhuparna Bhowmik <[email protected]>
>>>
>>> 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 <[email protected]>
>>> 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
On Wed, Jan 15, 2020 at 12:04:58AM +0530, [email protected] wrote:
> From: Dennis Dalessandro <[email protected]>
>
> 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,
> > > [email protected] wrote:
> > > > From: Madhuparna Bhowmik <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ? 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
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,
> > > [email protected] wrote:
> > > > From: Madhuparna Bhowmik <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > 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
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,
>>>> [email protected] wrote:
>>>>> From: Madhuparna Bhowmik <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> 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
On 2/14/2020 10:43 AM, Madhuparna Bhowmik wrote:
>
>
> On Wed, Jan 15, 2020 at 12:05 AM <[email protected]
> <mailto:[email protected]>> wrote:
>
> From: Dennis Dalessandro <[email protected]
> <mailto:[email protected]>>
>
> 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,
> >> [email protected]
> <mailto:[email protected]> wrote:
> >>> From: Madhuparna Bhowmik <[email protected]
> <mailto:[email protected]>>
> >>>
> >>> 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
> <[email protected] <mailto:[email protected]>>
> >>> 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