On Thu, Nov 22, 2018 at 08:50:00PM +0200, Leon Romanovsky wrote:
> On Thu, Nov 22, 2018 at 11:13:46AM -0700, Jason Gunthorpe wrote:
> > On Sat, Oct 27, 2018 at 03:15:18PM +0800, Lijun Ou wrote:
> > > +void hns_roce_srq_event(struct hns_roce_dev *hr_dev, u32 srqn, int event_type)
> > > +{
> > > + struct hns_roce_srq_table *srq_table = &hr_dev->srq_table;
> > > + struct hns_roce_srq *srq;
> > > +
> > > + rcu_read_lock();
> > > + srq = radix_tree_lookup(&srq_table->tree,
> > > + srqn & (hr_dev->caps.num_srqs - 1));
> > > + rcu_read_unlock();
> > > + if (srq) {
> > > + refcount_inc(&srq->refcount);
> >
> > This doesn't look right to me..
> >
> > The refcount_inc should be inside the RCU and it should almost
> > certainly be refcount_inc_not_zero.
>
> Are you sure that you can update the variable inside of struct which
> is protected by RCU?
If there is concurrency here then some locking is needed to make sure
srq is valid. Maybe it is not RCU, but then it is unclear why RCU is
being used here..
Most probably the existing spinlock around the radix tree should be
used here, not RCU.
Jason
