linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
@ 2020-01-07 17:35 madhuparnabhowmik04
  2020-01-07 17:35 ` [PATCH 2/3] infiniband: hw: qib: qib_verbs: " madhuparnabhowmik04
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: madhuparnabhowmik04 @ 2020-01-07 17:35 UTC (permalink / raw)
  To: dennis.dalessandro, mike.marciniszyn, dledford, paulmck
  Cc: rcu, joel, frextrite, linux-kernel-mentees, linux-rdma,
	linux-kernel, Madhuparna Bhowmik

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
index 089e201d7550..cab2ff185240 100644
--- a/drivers/infiniband/hw/hfi1/verbs.c
+++ b/drivers/infiniband/hw/hfi1/verbs.c
@@ -515,7 +515,8 @@ 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
+								lock_is_held(&(ibp->rvp.lock).dep_map)) {
 			packet->qp = p->qp;
 			if (hfi1_do_pkey_check(packet))
 				goto drop;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/3] infiniband: hw: qib: qib_verbs: Use built-in RCU list checking
  2020-01-07 17:35 [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking madhuparnabhowmik04
@ 2020-01-07 17:35 ` madhuparnabhowmik04
  2020-01-07 17:35 ` [PATCH 3/3] infiniband: sw: rdmavt: mcast.c: " madhuparnabhowmik04
  2020-01-07 18:26 ` [PATCH 1/3] infiniband: hw: hfi1: verbs.c: " Jason Gunthorpe
  2 siblings, 0 replies; 16+ messages in thread
From: madhuparnabhowmik04 @ 2020-01-07 17:35 UTC (permalink / raw)
  To: dennis.dalessandro, mike.marciniszyn, dledford, paulmck
  Cc: rcu, joel, frextrite, linux-kernel-mentees, linux-rdma,
	linux-kernel, Madhuparna Bhowmik

From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>

Use built-in RCU and lock checking for list_for_each_entry_rcu
by passing cond argument to it.

Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
---
 drivers/infiniband/hw/qib/qib_verbs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/qib/qib_verbs.c b/drivers/infiniband/hw/qib/qib_verbs.c
index 33778d451b82..4dc7514ce626 100644
--- a/drivers/infiniband/hw/qib/qib_verbs.c
+++ b/drivers/infiniband/hw/qib/qib_verbs.c
@@ -329,7 +329,8 @@ void qib_ib_rcv(struct qib_ctxtdata *rcd, void *rhdr, void *data, u32 tlen)
 		if (mcast == NULL)
 			goto drop;
 		this_cpu_inc(ibp->pmastats->n_multicast_rcv);
-		list_for_each_entry_rcu(p, &mcast->qp_list, list)
+		list_for_each_entry_rcu(p, &mcast->qp_list, list
+								lock_is_held(&(ibp->rvp.lock).dep_map))
 			qib_qp_rcv(rcd, hdr, 1, data, tlen, p->qp);
 		/*
 		 * Notify rvt_multicast_detach() if it is waiting for us
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/3] infiniband: sw: rdmavt: mcast.c: Use built-in RCU list checking
  2020-01-07 17:35 [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking madhuparnabhowmik04
  2020-01-07 17:35 ` [PATCH 2/3] infiniband: hw: qib: qib_verbs: " madhuparnabhowmik04
@ 2020-01-07 17:35 ` madhuparnabhowmik04
  2020-01-07 18:26 ` [PATCH 1/3] infiniband: hw: hfi1: verbs.c: " Jason Gunthorpe
  2 siblings, 0 replies; 16+ messages in thread
From: madhuparnabhowmik04 @ 2020-01-07 17:35 UTC (permalink / raw)
  To: dennis.dalessandro, mike.marciniszyn, dledford, paulmck
  Cc: rcu, joel, frextrite, linux-kernel-mentees, linux-rdma,
	linux-kernel, Madhuparna Bhowmik

From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>

Use built-in RCU and lock-checking for list_for_each_entry_rcu()
by passing the cond argument.

Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com>
---
 drivers/infiniband/sw/rdmavt/mcast.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/rdmavt/mcast.c b/drivers/infiniband/sw/rdmavt/mcast.c
index dd11c6fcd060..5fce375f22f4 100644
--- a/drivers/infiniband/sw/rdmavt/mcast.c
+++ b/drivers/infiniband/sw/rdmavt/mcast.c
@@ -224,7 +224,8 @@ static int rvt_mcast_add(struct rvt_dev_info *rdi, struct rvt_ibport *ibp,
 		}
 
 		/* Search the QP list to see if this is already there. */
-		list_for_each_entry_rcu(p, &tmcast->qp_list, list) {
+		list_for_each_entry_rcu(p, &tmcast->qp_list, list
+								lock_is_held(&(ibp->lock).dep_map)) {
 			if (p->qp == mqp->qp) {
 				ret = ESRCH;
 				goto bail;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
  2020-01-07 17:35 [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking madhuparnabhowmik04
  2020-01-07 17:35 ` [PATCH 2/3] infiniband: hw: qib: qib_verbs: " madhuparnabhowmik04
  2020-01-07 17:35 ` [PATCH 3/3] infiniband: sw: rdmavt: mcast.c: " madhuparnabhowmik04
@ 2020-01-07 18:26 ` Jason Gunthorpe
  2 siblings, 0 replies; 16+ messages in thread
From: Jason Gunthorpe @ 2020-01-07 18:26 UTC (permalink / raw)
  To: madhuparnabhowmik04
  Cc: dennis.dalessandro, mike.marciniszyn, dledford, paulmck, rcu,
	joel, frextrite, linux-kernel-mentees, linux-rdma, linux-kernel

On Tue, Jan 07, 2020 at 11:05:08PM +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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
> index 089e201d7550..cab2ff185240 100644
> +++ b/drivers/infiniband/hw/hfi1/verbs.c
> @@ -515,7 +515,8 @@ 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
> +								lock_is_held(&(ibp->rvp.lock).dep_map)) {

This is missing a ',' and isn't indented properly. Does it even
compile?

The idea seems sound though.

Jason

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
       [not found]       ` <CAF65HP0RsW5FMRRf5Lia2=MTKex-KwO7-_NsCUef94YKBg+pfA@mail.gmail.com>
@ 2020-02-14 17:24         ` Dennis Dalessandro
  0 siblings, 0 replies; 16+ 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] 16+ messages in thread

* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
  2020-01-14 19:41       ` Jason Gunthorpe
@ 2020-01-14 19:46         ` Dennis Dalessandro
  0 siblings, 0 replies; 16+ 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] 16+ 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
@ 2020-01-14 19:41       ` Jason Gunthorpe
  2020-01-14 19:46         ` Dennis Dalessandro
       [not found]       ` <CAF65HP0RsW5FMRRf5Lia2=MTKex-KwO7-_NsCUef94YKBg+pfA@mail.gmail.com>
  2 siblings, 1 reply; 16+ 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] 16+ 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
  2020-01-14 19:41       ` Jason Gunthorpe
       [not found]       ` <CAF65HP0RsW5FMRRf5Lia2=MTKex-KwO7-_NsCUef94YKBg+pfA@mail.gmail.com>
  2 siblings, 0 replies; 16+ 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] 16+ messages in thread

* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
  2020-01-14 17:00   ` Dennis Dalessandro
@ 2020-01-14 18:24     ` Dennis Dalessandro
  2020-01-14 19:17       ` Joel Fernandes
                         ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Dennis Dalessandro @ 2020-01-14 18:24 UTC (permalink / raw)
  To: Jason Gunthorpe, madhuparnabhowmik04
  Cc: mike.marciniszyn, paulmck, joel, frextrite, linux-kernel-mentees,
	rcu, linux-rdma, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
  2020-01-14 16:57 ` Jason Gunthorpe
@ 2020-01-14 17:00   ` Dennis Dalessandro
  2020-01-14 18:24     ` Dennis Dalessandro
  0 siblings, 1 reply; 16+ messages in thread
From: Dennis Dalessandro @ 2020-01-14 17:00 UTC (permalink / raw)
  To: Jason Gunthorpe, madhuparnabhowmik04
  Cc: mike.marciniszyn, paulmck, joel, frextrite, linux-kernel-mentees,
	rcu, linux-rdma, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
  2020-01-14 16:23 madhuparnabhowmik04
@ 2020-01-14 16:57 ` Jason Gunthorpe
  2020-01-14 17:00   ` Dennis Dalessandro
  0 siblings, 1 reply; 16+ messages in thread
From: Jason Gunthorpe @ 2020-01-14 16:57 UTC (permalink / raw)
  To: madhuparnabhowmik04
  Cc: mike.marciniszyn, dennis.dalessandro, paulmck, joel, frextrite,
	linux-kernel-mentees, rcu, linux-rdma, linux-kernel

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?

Jason

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
@ 2020-01-14 16:23 madhuparnabhowmik04
  2020-01-14 16:57 ` Jason Gunthorpe
  0 siblings, 1 reply; 16+ messages in thread
From: madhuparnabhowmik04 @ 2020-01-14 16:23 UTC (permalink / raw)
  To: mike.marciniszyn, dennis.dalessandro, jgg, paulmck
  Cc: joel, frextrite, linux-kernel-mentees, rcu, linux-rdma,
	linux-kernel, Madhuparna Bhowmik

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
--- 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
  2020-01-07 20:33 ` Jason Gunthorpe
  2020-01-09 18:10   ` Jason Gunthorpe
@ 2020-01-10 15:54   ` Joel Fernandes
  1 sibling, 0 replies; 16+ messages in thread
From: Joel Fernandes @ 2020-01-10 15:54 UTC (permalink / raw)
  To: madhuparnabhowmik04
  Cc: jgg, dennis.dalessandro, mike.marciniszyn, dledford, paulmck,
	rcu, frextrite, linux-kernel-mentees, linux-rdma, linux-kernel

On Wed, Jan 08, 2020 at 01:35:07PM +0530, madhuparnabhowmik04@gmail.com wrote:
> From: Jason Gunthorpe <jgg@ziepe.ca>
> 
> On Wed, Jan 08, 2020 at 12:59:12AM +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 | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
> > index 089e201d7550..e6abdbcb4ffb 100644
> > +++ b/drivers/infiniband/hw/hfi1/verbs.c
> > @@ -515,7 +515,8 @@ 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,
> > +					lock_is_held(&(ibp->rvp.lock).dep_map)) {
> 
> Why .dep_map? Does this compile?

Yeah, have you really compiled this? Don't send patches without at least
compile testing !!

> Alternatively, it can be lockdep_is_held(ibp->rvp.lock).
> Please refer to the macro(link below) and let me know if the usage of lock_is_held()
> in the patch is correct.
> 
> https://elixir.bootlin.com/linux/v5.5-rc2/source/include/linux/lockdep.h#L364

Please use lockdep_is_held(). Thanks.

thanks,

 - Joel

> 
> Thanks,
> Madhuparna
> Jason

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
  2020-01-07 20:33 ` Jason Gunthorpe
@ 2020-01-09 18:10   ` Jason Gunthorpe
  2020-01-10 15:54   ` Joel Fernandes
  1 sibling, 0 replies; 16+ messages in thread
From: Jason Gunthorpe @ 2020-01-09 18:10 UTC (permalink / raw)
  To: madhuparnabhowmik04
  Cc: dennis.dalessandro, mike.marciniszyn, dledford, paulmck, rcu,
	joel, frextrite, linux-kernel-mentees, linux-rdma, linux-kernel

On Wed, Jan 08, 2020 at 01:38:50PM +0530, madhuparnabhowmik04@gmail.com wrote:

> Alternatively, it can be lockdep_is_held(ibp->rvp.lock).
> Please refer to the macro(link below) and let me know if the usage of lock_is_held()
> in the patch is correct.

lock_is_held is the normal way to write this

Jason

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
  2020-01-07 19:29 madhuparnabhowmik04
@ 2020-01-07 20:33 ` Jason Gunthorpe
  2020-01-09 18:10   ` Jason Gunthorpe
  2020-01-10 15:54   ` Joel Fernandes
  0 siblings, 2 replies; 16+ messages in thread
From: Jason Gunthorpe @ 2020-01-07 20:33 UTC (permalink / raw)
  To: madhuparnabhowmik04
  Cc: dennis.dalessandro, mike.marciniszyn, dledford, paulmck, rcu,
	joel, frextrite, linux-kernel-mentees, linux-rdma, linux-kernel

On Wed, Jan 08, 2020 at 12:59:12AM +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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
> index 089e201d7550..e6abdbcb4ffb 100644
> +++ b/drivers/infiniband/hw/hfi1/verbs.c
> @@ -515,7 +515,8 @@ 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,
> +					lock_is_held(&(ibp->rvp.lock).dep_map)) {

Why .dep_map? Does this compile?

Jason

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking
@ 2020-01-07 19:29 madhuparnabhowmik04
  2020-01-07 20:33 ` Jason Gunthorpe
  0 siblings, 1 reply; 16+ messages in thread
From: madhuparnabhowmik04 @ 2020-01-07 19:29 UTC (permalink / raw)
  To: dennis.dalessandro, mike.marciniszyn, dledford, paulmck
  Cc: rcu, joel, frextrite, linux-kernel-mentees, linux-rdma,
	linux-kernel, Madhuparna Bhowmik

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
index 089e201d7550..e6abdbcb4ffb 100644
--- a/drivers/infiniband/hw/hfi1/verbs.c
+++ b/drivers/infiniband/hw/hfi1/verbs.c
@@ -515,7 +515,8 @@ 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,
+					lock_is_held(&(ibp->rvp.lock).dep_map)) {
 			packet->qp = p->qp;
 			if (hfi1_do_pkey_check(packet))
 				goto drop;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-02-14 17:25 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-07 17:35 [PATCH 1/3] infiniband: hw: hfi1: verbs.c: Use built-in RCU list checking madhuparnabhowmik04
2020-01-07 17:35 ` [PATCH 2/3] infiniband: hw: qib: qib_verbs: " madhuparnabhowmik04
2020-01-07 17:35 ` [PATCH 3/3] infiniband: sw: rdmavt: mcast.c: " madhuparnabhowmik04
2020-01-07 18:26 ` [PATCH 1/3] infiniband: hw: hfi1: verbs.c: " Jason Gunthorpe
2020-01-07 19:29 madhuparnabhowmik04
2020-01-07 20:33 ` Jason Gunthorpe
2020-01-09 18:10   ` Jason Gunthorpe
2020-01-10 15:54   ` Joel Fernandes
2020-01-14 16:23 madhuparnabhowmik04
2020-01-14 16:57 ` Jason Gunthorpe
2020-01-14 17:00   ` Dennis Dalessandro
2020-01-14 18:24     ` Dennis Dalessandro
2020-01-14 19:17       ` Joel Fernandes
2020-01-14 19:41       ` Jason Gunthorpe
2020-01-14 19:46         ` Dennis Dalessandro
     [not found]       ` <CAF65HP0RsW5FMRRf5Lia2=MTKex-KwO7-_NsCUef94YKBg+pfA@mail.gmail.com>
2020-02-14 17:24         ` Dennis Dalessandro

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).