| Message ID | 20181218050334.13459-1-yanjun.zhu@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [1/1] IB: rxe: replace read/write locks with atomic bitops | expand |
On 12/17/18 9:03 PM, Zhu Yanjun wrote: > In rxe_pool.c, pool state is protected by some read/write locks. > Now these read/write locks are replaced with atomic bitops. This can > make source code compact. I don't think that making the source code more compact is sufficient as a motivation for this change. Unless a better motivation can be provided I prefer to keep the current code. Thanks, Bart.
On 2018/12/18 13:13, Bart Van Assche wrote: > On 12/17/18 9:03 PM, Zhu Yanjun wrote: >> In rxe_pool.c, pool state is protected by some read/write locks. >> Now these read/write locks are replaced with atomic bitops. This can >> make source code compact. > > I don't think that making the source code more compact is sufficient > as a motivation for this change. I think atomic bitops is a common method to the state. Zhu Yanjun > Unless a better motivation can be provided I prefer to keep the > current code. > > Thanks, > > Bart.
On Tue, Dec 18, 2018 at 12:03:34AM -0500, Zhu Yanjun wrote: > In rxe_pool.c, pool state is protected by some read/write locks. > Now these read/write locks are replaced with atomic bitops. This can > make source code compact. > > Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> > drivers/infiniband/sw/rxe/rxe_pool.c | 22 +++++++--------------- > drivers/infiniband/sw/rxe/rxe_pool.h | 2 +- > 2 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c > index 36b53fb94a49..1a7efe0e3bb1 100644 > +++ b/drivers/infiniband/sw/rxe/rxe_pool.c > @@ -222,7 +222,7 @@ int rxe_pool_init( > pool->key_size = rxe_type_info[type].key_size; > } > > - pool->state = RXE_POOL_STATE_VALID; > + set_bit(RXE_POOL_STATE_VALID, &pool->state); > > out: > return err; > @@ -232,7 +232,7 @@ static void rxe_pool_release(struct kref *kref) > { > struct rxe_pool *pool = container_of(kref, struct rxe_pool, ref_cnt); > > - pool->state = RXE_POOL_STATE_INVALID; > + set_bit(RXE_POOL_STATE_INVALID, &pool->state); A bit called valid and a bit called invalid at the same time?? What is wrong with set_bit/clear_bit ??? Jason
On Tue, Dec 18, 2018 at 12:03:34AM -0500, Zhu Yanjun wrote: > In rxe_pool.c, pool state is protected by some read/write locks. > Now these read/write locks are replaced with atomic bitops. This can > make source code compact. > > Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> > drivers/infiniband/sw/rxe/rxe_pool.c | 22 +++++++--------------- > drivers/infiniband/sw/rxe/rxe_pool.h | 2 +- > 2 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c > index 36b53fb94a49..1a7efe0e3bb1 100644 > +++ b/drivers/infiniband/sw/rxe/rxe_pool.c > @@ -222,7 +222,7 @@ int rxe_pool_init( > pool->key_size = rxe_type_info[type].key_size; > } > > - pool->state = RXE_POOL_STATE_VALID; > + set_bit(RXE_POOL_STATE_VALID, &pool->state); > > out: > return err; > @@ -232,7 +232,7 @@ static void rxe_pool_release(struct kref *kref) > { > struct rxe_pool *pool = container_of(kref, struct rxe_pool, ref_cnt); > > - pool->state = RXE_POOL_STATE_INVALID; > + set_bit(RXE_POOL_STATE_INVALID, &pool->state); > kfree(pool->table); > } > > @@ -243,14 +243,10 @@ static void rxe_pool_put(struct rxe_pool *pool) > > int rxe_pool_cleanup(struct rxe_pool *pool) > { > - unsigned long flags; > - > - write_lock_irqsave(&pool->pool_lock, flags); > - pool->state = RXE_POOL_STATE_INVALID; > + set_bit(RXE_POOL_STATE_INVALID, &pool->state); > if (atomic_read(&pool->num_elem) > 0) > pr_warn("%s pool destroyed with unfree'd elem\n", > pool_name(pool)); > - write_unlock_irqrestore(&pool->pool_lock, flags); > > rxe_pool_put(pool); > > @@ -380,17 +376,13 @@ void rxe_drop_index(void *arg) > void *rxe_alloc(struct rxe_pool *pool) > { > struct rxe_pool_entry *elem; > - unsigned long flags; > > might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC)); > > - read_lock_irqsave(&pool->pool_lock, flags); > - if (pool->state != RXE_POOL_STATE_VALID) { > - read_unlock_irqrestore(&pool->pool_lock, flags); > + if (!test_bit(RXE_POOL_STATE_VALID, &pool->state)) > return NULL; > - } > + > kref_get(&pool->ref_cnt); > - read_unlock_irqrestore(&pool->pool_lock, flags); I doubt this locking can be removed without creating races between pool_state and the kref_get. I suspect the locking, refcounting, and lifetime in this pool code is just all wrong. If you fix that then the pool->state probably just goes away completely. Good code usually does't have racy coding patterns like the above at least.. Jason
On 2018/12/19 0:42, Jason Gunthorpe wrote: > On Tue, Dec 18, 2018 at 12:03:34AM -0500, Zhu Yanjun wrote: >> In rxe_pool.c, pool state is protected by some read/write locks. >> Now these read/write locks are replaced with atomic bitops. This can >> make source code compact. >> >> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> >> drivers/infiniband/sw/rxe/rxe_pool.c | 22 +++++++--------------- >> drivers/infiniband/sw/rxe/rxe_pool.h | 2 +- >> 2 files changed, 8 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c >> index 36b53fb94a49..1a7efe0e3bb1 100644 >> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c >> @@ -222,7 +222,7 @@ int rxe_pool_init( >> pool->key_size = rxe_type_info[type].key_size; >> } >> >> - pool->state = RXE_POOL_STATE_VALID; >> + set_bit(RXE_POOL_STATE_VALID, &pool->state); >> >> out: >> return err; >> @@ -232,7 +232,7 @@ static void rxe_pool_release(struct kref *kref) >> { >> struct rxe_pool *pool = container_of(kref, struct rxe_pool, ref_cnt); >> >> - pool->state = RXE_POOL_STATE_INVALID; >> + set_bit(RXE_POOL_STATE_INVALID, &pool->state); >> kfree(pool->table); >> } >> >> @@ -243,14 +243,10 @@ static void rxe_pool_put(struct rxe_pool *pool) >> >> int rxe_pool_cleanup(struct rxe_pool *pool) >> { >> - unsigned long flags; >> - >> - write_lock_irqsave(&pool->pool_lock, flags); >> - pool->state = RXE_POOL_STATE_INVALID; >> + set_bit(RXE_POOL_STATE_INVALID, &pool->state); >> if (atomic_read(&pool->num_elem) > 0) >> pr_warn("%s pool destroyed with unfree'd elem\n", >> pool_name(pool)); >> - write_unlock_irqrestore(&pool->pool_lock, flags); >> >> rxe_pool_put(pool); >> >> @@ -380,17 +376,13 @@ void rxe_drop_index(void *arg) >> void *rxe_alloc(struct rxe_pool *pool) >> { >> struct rxe_pool_entry *elem; >> - unsigned long flags; >> >> might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC)); >> >> - read_lock_irqsave(&pool->pool_lock, flags); >> - if (pool->state != RXE_POOL_STATE_VALID) { >> - read_unlock_irqrestore(&pool->pool_lock, flags); >> + if (!test_bit(RXE_POOL_STATE_VALID, &pool->state)) >> return NULL; >> - } >> + >> kref_get(&pool->ref_cnt); >> - read_unlock_irqrestore(&pool->pool_lock, flags); > I doubt this locking can be removed without creating races between > pool_state and the kref_get. > > I suspect the locking, refcounting, and lifetime in this pool code is > just all wrong. If you fix that then the pool->state probably just > goes away completely. Thanks. The pool->state is the duplicate of ref_cnt. ref_cnt = 0 is the same with pool->state = RXE_POOL_STATE_INVALID ref_cnt > 0 is the same with pool->state = RXE_POOL_STATE_VALID. So we can use ref_cn to replace pool->state. Thanks for your suggestions. I will send V2 soon. Zhu Yanjun > > Good code usually does't have racy coding patterns like the above at > least.. > > Jason >
On 2018/12/19 13:22, Yanjun Zhu wrote: > > On 2018/12/19 0:42, Jason Gunthorpe wrote: >> On Tue, Dec 18, 2018 at 12:03:34AM -0500, Zhu Yanjun wrote: >>> In rxe_pool.c, pool state is protected by some read/write locks. >>> Now these read/write locks are replaced with atomic bitops. This can >>> make source code compact. >>> >>> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> >>> drivers/infiniband/sw/rxe/rxe_pool.c | 22 +++++++--------------- >>> drivers/infiniband/sw/rxe/rxe_pool.h | 2 +- >>> 2 files changed, 8 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c >>> b/drivers/infiniband/sw/rxe/rxe_pool.c >>> index 36b53fb94a49..1a7efe0e3bb1 100644 >>> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c >>> @@ -222,7 +222,7 @@ int rxe_pool_init( >>> pool->key_size = rxe_type_info[type].key_size; >>> } >>> - pool->state = RXE_POOL_STATE_VALID; >>> + set_bit(RXE_POOL_STATE_VALID, &pool->state); >>> out: >>> return err; >>> @@ -232,7 +232,7 @@ static void rxe_pool_release(struct kref *kref) >>> { >>> struct rxe_pool *pool = container_of(kref, struct rxe_pool, >>> ref_cnt); >>> - pool->state = RXE_POOL_STATE_INVALID; >>> + set_bit(RXE_POOL_STATE_INVALID, &pool->state); >>> kfree(pool->table); >>> } >>> @@ -243,14 +243,10 @@ static void rxe_pool_put(struct rxe_pool *pool) >>> int rxe_pool_cleanup(struct rxe_pool *pool) >>> { >>> - unsigned long flags; >>> - >>> - write_lock_irqsave(&pool->pool_lock, flags); >>> - pool->state = RXE_POOL_STATE_INVALID; >>> + set_bit(RXE_POOL_STATE_INVALID, &pool->state); >>> if (atomic_read(&pool->num_elem) > 0) >>> pr_warn("%s pool destroyed with unfree'd elem\n", >>> pool_name(pool)); >>> - write_unlock_irqrestore(&pool->pool_lock, flags); >>> rxe_pool_put(pool); >>> @@ -380,17 +376,13 @@ void rxe_drop_index(void *arg) >>> void *rxe_alloc(struct rxe_pool *pool) >>> { >>> struct rxe_pool_entry *elem; >>> - unsigned long flags; >>> might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC)); >>> - read_lock_irqsave(&pool->pool_lock, flags); >>> - if (pool->state != RXE_POOL_STATE_VALID) { >>> - read_unlock_irqrestore(&pool->pool_lock, flags); >>> + if (!test_bit(RXE_POOL_STATE_VALID, &pool->state)) >>> return NULL; >>> - } >>> + >>> kref_get(&pool->ref_cnt); >>> - read_unlock_irqrestore(&pool->pool_lock, flags); >> I doubt this locking can be removed without creating races between >> pool_state and the kref_get. >> >> I suspect the locking, refcounting, and lifetime in this pool code is >> just all wrong. If you fix that then the pool->state probably just >> goes away completely. Thanks. Jason. Now V2 patch is sent. Please check it. I made simple tests with Patch v2. And I can not find any bug. If there is something wrong this Patch v2, please let me know. Zhu Yanjun > > Thanks. The pool->state is the duplicate of ref_cnt. > > ref_cnt = 0 is the same with pool->state = RXE_POOL_STATE_INVALID > > ref_cnt > 0 is the same with pool->state = RXE_POOL_STATE_VALID. > > So we can use ref_cn to replace pool->state. > > Thanks for your suggestions. > > I will send V2 soon. > > Zhu Yanjun > >> >> Good code usually does't have racy coding patterns like the above at >> least.. >> >> Jason >> >
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c index 36b53fb94a49..1a7efe0e3bb1 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.c +++ b/drivers/infiniband/sw/rxe/rxe_pool.c @@ -222,7 +222,7 @@ int rxe_pool_init( pool->key_size = rxe_type_info[type].key_size; } - pool->state = RXE_POOL_STATE_VALID; + set_bit(RXE_POOL_STATE_VALID, &pool->state); out: return err; @@ -232,7 +232,7 @@ static void rxe_pool_release(struct kref *kref) { struct rxe_pool *pool = container_of(kref, struct rxe_pool, ref_cnt); - pool->state = RXE_POOL_STATE_INVALID; + set_bit(RXE_POOL_STATE_INVALID, &pool->state); kfree(pool->table); } @@ -243,14 +243,10 @@ static void rxe_pool_put(struct rxe_pool *pool) int rxe_pool_cleanup(struct rxe_pool *pool) { - unsigned long flags; - - write_lock_irqsave(&pool->pool_lock, flags); - pool->state = RXE_POOL_STATE_INVALID; + set_bit(RXE_POOL_STATE_INVALID, &pool->state); if (atomic_read(&pool->num_elem) > 0) pr_warn("%s pool destroyed with unfree'd elem\n", pool_name(pool)); - write_unlock_irqrestore(&pool->pool_lock, flags); rxe_pool_put(pool); @@ -380,17 +376,13 @@ void rxe_drop_index(void *arg) void *rxe_alloc(struct rxe_pool *pool) { struct rxe_pool_entry *elem; - unsigned long flags; might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC)); - read_lock_irqsave(&pool->pool_lock, flags); - if (pool->state != RXE_POOL_STATE_VALID) { - read_unlock_irqrestore(&pool->pool_lock, flags); + if (!test_bit(RXE_POOL_STATE_VALID, &pool->state)) return NULL; - } + kref_get(&pool->ref_cnt); - read_unlock_irqrestore(&pool->pool_lock, flags); kref_get(&pool->rxe->ref_cnt); @@ -438,7 +430,7 @@ void *rxe_pool_get_index(struct rxe_pool *pool, u32 index) read_lock_irqsave(&pool->pool_lock, flags); - if (pool->state != RXE_POOL_STATE_VALID) + if (!test_bit(RXE_POOL_STATE_VALID, &pool->state)) goto out; node = pool->tree.rb_node; @@ -470,7 +462,7 @@ void *rxe_pool_get_key(struct rxe_pool *pool, void *key) read_lock_irqsave(&pool->pool_lock, flags); - if (pool->state != RXE_POOL_STATE_VALID) + if (!test_bit(RXE_POOL_STATE_VALID, &pool->state)) goto out; node = pool->tree.rb_node; diff --git a/drivers/infiniband/sw/rxe/rxe_pool.h b/drivers/infiniband/sw/rxe/rxe_pool.h index aa4ba307097b..5cc6b01c274d 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.h +++ b/drivers/infiniband/sw/rxe/rxe_pool.h @@ -94,7 +94,7 @@ struct rxe_pool { size_t elem_size; struct kref ref_cnt; void (*cleanup)(struct rxe_pool_entry *obj); - enum rxe_pool_state state; + unsigned long state; enum rxe_pool_flags flags; enum rxe_elem_type type;
In rxe_pool.c, pool state is protected by some read/write locks. Now these read/write locks are replaced with atomic bitops. This can make source code compact. Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> --- drivers/infiniband/sw/rxe/rxe_pool.c | 22 +++++++--------------- drivers/infiniband/sw/rxe/rxe_pool.h | 2 +- 2 files changed, 8 insertions(+), 16 deletions(-)