On Thu, Nov 22, 2018 at 11:11:00AM -0800, Matthew Wilcox wrote:
> 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?
>
> Yes. Holding the rcu_read_lock() prevents the structure from being freed
> after its refcount hits zero. refcount_inc_not_zero() will fail if the
> structure is waiting to be freed.
Matthew,
In your answer, you rely on the assumption that they use refcount for
free paths only. I'm not so sure.
Thanks
>
Attachment:
signature.asc
Description: PGP signature
