| Message ID | 20190221002107.22625-20-willy@infradead.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Convert the Infiniband subsystem to XArray | expand |
> -----Original Message----- > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > owner@vger.kernel.org> On Behalf Of Matthew Wilcox > Sent: Wednesday, February 20, 2019 6:21 PM > To: Jason Gunthorpe <jgg@ziepe.ca> > Cc: Matthew Wilcox <willy@infradead.org>; linux-rdma@vger.kernel.org > Subject: [PATCH 19/32] cxgb4: Convert atid_idr to XArray > > Signed-off-by: Matthew Wilcox <willy@infradead.org> > --- > drivers/infiniband/hw/cxgb4/cm.c | 25 +++++++++++++++---------- > drivers/infiniband/hw/cxgb4/device.c | 16 +++++++--------- > drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 2 +- > 3 files changed, 23 insertions(+), 20 deletions(-) > > diff --git a/drivers/infiniband/hw/cxgb4/cm.c > b/drivers/infiniband/hw/cxgb4/cm.c > index b31adec05009..3a0d4ce471c8 100644 > --- a/drivers/infiniband/hw/cxgb4/cm.c > +++ b/drivers/infiniband/hw/cxgb4/cm.c > @@ -558,7 +558,7 @@ static void act_open_req_arp_failure(void *handle, > struct sk_buff *skb) > cxgb4_clip_release(ep->com.dev->rdev.lldi.ports[0], > (const u32 *)&sin6->sin6_addr.s6_addr, 1); > } > - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, ep->atid); > + xa_erase_irq(&ep->com.dev->atids, ep->atid); > cxgb4_free_atid(ep->com.dev->rdev.lldi.tids, ep->atid); > queue_arp_failure_cpl(ep, skb, FAKE_CPL_PUT_EP_SAFE); > } > @@ -1201,7 +1201,7 @@ static int act_establish(struct c4iw_dev *dev, struct > sk_buff *skb) > set_emss(ep, tcp_opt); > > /* dealloc the atid */ > - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, atid); > + xa_erase_irq(&ep->com.dev->atids, atid); > cxgb4_free_atid(t, atid); > set_bit(ACT_ESTAB, &ep->com.history); > > @@ -2147,7 +2147,9 @@ static int c4iw_reconnect(struct c4iw_ep *ep) > err = -ENOMEM; > goto fail2; > } > - insert_handle(ep->com.dev, &ep->com.dev->atid_idr, ep, ep->atid); > + err = xa_insert_irq(&ep->com.dev->atids, ep->atid, ep, > GFP_KERNEL); > + if (err) > + goto fail5; > > /* find a route */ > if (ep->com.cm_id->m_local_addr.ss_family == AF_INET) { > @@ -2198,7 +2200,8 @@ static int c4iw_reconnect(struct c4iw_ep *ep) > fail4: > dst_release(ep->dst); > fail3: > - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, ep->atid); > + xa_erase_irq(&ep->com.dev->atids, ep->atid); > +fail5: > cxgb4_free_atid(ep->com.dev->rdev.lldi.tids, ep->atid); > fail2: Don't like the out-of-order label. Care to rename all of them to make them more descriptive and avoid this 4, 3, 5, 2 sequence? > /* > @@ -2281,8 +2284,7 @@ static int act_open_rpl(struct c4iw_dev *dev, struct > sk_buff *skb) > (const u32 *) > &sin6->sin6_addr.s6_addr, > 1); > } > - remove_handle(ep->com.dev, &ep->com.dev- > >atid_idr, > - atid); > + xa_erase_irq(&ep->com.dev->atids, atid); > cxgb4_free_atid(t, atid); > dst_release(ep->dst); > cxgb4_l2t_release(ep->l2t); > @@ -2319,7 +2321,7 @@ static int act_open_rpl(struct c4iw_dev *dev, struct > sk_buff *skb) > cxgb4_remove_tid(ep->com.dev->rdev.lldi.tids, 0, > GET_TID(rpl), > ep->com.local_addr.ss_family); > > - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, atid); > + xa_erase_irq(&ep->com.dev->atids, atid); > cxgb4_free_atid(t, atid); > dst_release(ep->dst); > cxgb4_l2t_release(ep->l2t); > @@ -3265,7 +3267,9 @@ int c4iw_connect(struct iw_cm_id *cm_id, struct > iw_cm_conn_param *conn_param) > err = -ENOMEM; > goto fail2; > } > - insert_handle(dev, &dev->atid_idr, ep, ep->atid); > + err = xa_insert_irq(&dev->atids, ep->atid, ep, GFP_KERNEL); > + if (err) > + goto fail5; > > memcpy(&ep->com.local_addr, &cm_id->m_local_addr, > sizeof(ep->com.local_addr)); > @@ -3353,7 +3357,8 @@ int c4iw_connect(struct iw_cm_id *cm_id, struct > iw_cm_conn_param *conn_param) > fail4: > dst_release(ep->dst); > fail3: > - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, ep->atid); > + xa_erase_irq(&ep->com.dev->atids, ep->atid); > +fail5: > cxgb4_free_atid(ep->com.dev->rdev.lldi.tids, ep->atid); > fail2: > skb_queue_purge(&ep->com.ep_skb_list); Same comment. > @@ -3687,7 +3692,7 @@ static void active_ofld_conn_reply(struct c4iw_dev > *dev, struct sk_buff *skb, > cxgb4_clip_release(ep->com.dev->rdev.lldi.ports[0], > (const u32 *)&sin6->sin6_addr.s6_addr, 1); > } > - remove_handle(dev, &dev->atid_idr, atid); > + xa_erase_irq(&dev->atids, atid); > cxgb4_free_atid(dev->rdev.lldi.tids, atid); > dst_release(ep->dst); > cxgb4_l2t_release(ep->l2t); > diff --git a/drivers/infiniband/hw/cxgb4/device.c > b/drivers/infiniband/hw/cxgb4/device.c > index 7fae6e68c8f4..0bfba18a12d3 100644 > --- a/drivers/infiniband/hw/cxgb4/device.c > +++ b/drivers/infiniband/hw/cxgb4/device.c > @@ -617,11 +617,6 @@ static int dump_ep(struct c4iw_ep *ep, struct > c4iw_debugfs_data *epd) > return 0; > } > > -static int _dump_ep(int id, void *p, void *data) > -{ > - return dump_ep(p, data); > -} > - > static int dump_listen_ep(int id, void *p, void *data) > { > struct c4iw_listen_ep *ep = p; > @@ -695,8 +690,9 @@ static int ep_open(struct inode *inode, struct file > *file) > > xa_for_each(&epd->devp->hwtids, index, ep) > count++; > + xa_for_each(&epd->devp->atids, index, ep) > + count++; > spin_lock_irq(&epd->devp->lock); > - idr_for_each(&epd->devp->atid_idr, count_idrs, &count); > idr_for_each(&epd->devp->stid_idr, count_idrs, &count); > spin_unlock_irq(&epd->devp->lock); > > @@ -711,8 +707,11 @@ static int ep_open(struct inode *inode, struct file > *file) > xa_for_each(&epd->devp->hwtids, index, ep) > dump_ep(ep, epd); > xa_unlock_irq(&epd->devp->hwtids); > + xa_lock_irq(&epd->devp->atids); > + xa_for_each(&epd->devp->atids, index, ep) > + dump_ep(ep, epd); > + xa_unlock_irq(&epd->devp->atids); > spin_lock_irq(&epd->devp->lock); > - idr_for_each(&epd->devp->atid_idr, _dump_ep, epd); > idr_for_each(&epd->devp->stid_idr, dump_listen_ep, epd); > spin_unlock_irq(&epd->devp->lock); > > @@ -937,7 +936,6 @@ void c4iw_dealloc(struct uld_ctx *ctx) > c4iw_rdev_close(&ctx->dev->rdev); > wait_event(ctx->dev->wait, xa_empty(&ctx->dev->hwtids)); > idr_destroy(&ctx->dev->stid_idr); > - idr_destroy(&ctx->dev->atid_idr); > if (ctx->dev->rdev.bar2_kva) > iounmap(ctx->dev->rdev.bar2_kva); > if (ctx->dev->rdev.oc_mw_kva) > @@ -1045,8 +1043,8 @@ static struct c4iw_dev *c4iw_alloc(const struct > cxgb4_lld_info *infop) > xa_init_flags(&devp->qps, XA_FLAGS_LOCK_IRQ); > xa_init_flags(&devp->mrs, XA_FLAGS_LOCK_IRQ); > xa_init_flags(&devp->hwtids, XA_FLAGS_LOCK_IRQ); > + xa_init_flags(&devp->atids, XA_FLAGS_LOCK_IRQ); > idr_init(&devp->stid_idr); > - idr_init(&devp->atid_idr); > spin_lock_init(&devp->lock); > mutex_init(&devp->rdev.stats.lock); > mutex_init(&devp->db_mutex); > diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h > b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h > index 37003c3b8d51..6ff687f32249 100644 > --- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h > +++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h > @@ -323,7 +323,7 @@ struct c4iw_dev { > struct dentry *debugfs_root; > enum db_state db_state; > struct xarray hwtids; > - struct idr atid_idr; > + struct xarray atids; > struct idr stid_idr; > struct list_head db_fc_list; > u32 avail_ird; > -- > 2.20.1
On Thu, Feb 21, 2019 at 01:45:26PM -0600, Steve Wise wrote: > > @@ -2198,7 +2200,8 @@ static int c4iw_reconnect(struct c4iw_ep *ep) > > fail4: > > dst_release(ep->dst); > > fail3: > > - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, ep->atid); > > + xa_erase_irq(&ep->com.dev->atids, ep->atid); > > +fail5: > > cxgb4_free_atid(ep->com.dev->rdev.lldi.tids, ep->atid); > > fail2: > > Don't like the out-of-order label. Care to rename all of them to make them > more descriptive and avoid this 4, 3, 5, 2 sequence? I think the general kernel advice is to not number labels in the first place, for exactly this maintenance reason. Probably the best is a fail3_xa or something? Jason
On Thu, Feb 21, 2019 at 01:45:26PM -0600, Steve Wise wrote: > > @@ -2147,7 +2147,9 @@ static int c4iw_reconnect(struct c4iw_ep *ep) > > err = -ENOMEM; > > goto fail2; > > } > > - insert_handle(ep->com.dev, &ep->com.dev->atid_idr, ep, ep->atid); > > + err = xa_insert_irq(&ep->com.dev->atids, ep->atid, ep, > > GFP_KERNEL); > > + if (err) > > + goto fail5; > > > > /* find a route */ > > if (ep->com.cm_id->m_local_addr.ss_family == AF_INET) { > > @@ -2198,7 +2200,8 @@ static int c4iw_reconnect(struct c4iw_ep *ep) > > fail4: > > dst_release(ep->dst); > > fail3: > > - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, ep->atid); > > + xa_erase_irq(&ep->com.dev->atids, ep->atid); > > +fail5: > > cxgb4_free_atid(ep->com.dev->rdev.lldi.tids, ep->atid); > > fail2: > > Don't like the out-of-order label. Care to rename all of them to make them > more descriptive and avoid this 4, 3, 5, 2 sequence? That seemed incredibly dangerous to me. I'm not sure I could get it right. How about I introduce this one with a name, like most drivers do?
> -----Original Message----- > From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Thursday, February 21, 2019 1:51 PM > To: Steve Wise <swise@opengridcomputing.com> > Cc: 'Matthew Wilcox' <willy@infradead.org>; linux-rdma@vger.kernel.org > Subject: Re: [PATCH 19/32] cxgb4: Convert atid_idr to XArray > > On Thu, Feb 21, 2019 at 01:45:26PM -0600, Steve Wise wrote: > > > @@ -2198,7 +2200,8 @@ static int c4iw_reconnect(struct c4iw_ep *ep) > > > fail4: > > > dst_release(ep->dst); > > > fail3: > > > - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, ep->atid); > > > + xa_erase_irq(&ep->com.dev->atids, ep->atid); > > > +fail5: > > > cxgb4_free_atid(ep->com.dev->rdev.lldi.tids, ep->atid); > > > fail2: > > > > Don't like the out-of-order label. Care to rename all of them to make them > > more descriptive and avoid this 4, 3, 5, 2 sequence? > > I think the general kernel advice is to not number labels in the first > place, for exactly this maintenance reason. > > Probably the best is a fail3_xa or something? > > Jason Agreed. That is what I meant by "rename them all to make them more descriptive"...
> -----Original Message----- > From: Matthew Wilcox <willy@infradead.org> > Sent: Thursday, February 21, 2019 1:52 PM > To: Steve Wise <swise@opengridcomputing.com> > Cc: 'Jason Gunthorpe' <jgg@ziepe.ca>; linux-rdma@vger.kernel.org > Subject: Re: [PATCH 19/32] cxgb4: Convert atid_idr to XArray > > On Thu, Feb 21, 2019 at 01:45:26PM -0600, Steve Wise wrote: > > > @@ -2147,7 +2147,9 @@ static int c4iw_reconnect(struct c4iw_ep *ep) > > > err = -ENOMEM; > > > goto fail2; > > > } > > > - insert_handle(ep->com.dev, &ep->com.dev->atid_idr, ep, ep->atid); > > > + err = xa_insert_irq(&ep->com.dev->atids, ep->atid, ep, > > > GFP_KERNEL); > > > + if (err) > > > + goto fail5; > > > > > > /* find a route */ > > > if (ep->com.cm_id->m_local_addr.ss_family == AF_INET) { > > > @@ -2198,7 +2200,8 @@ static int c4iw_reconnect(struct c4iw_ep *ep) > > > fail4: > > > dst_release(ep->dst); > > > fail3: > > > - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, ep->atid); > > > + xa_erase_irq(&ep->com.dev->atids, ep->atid); > > > +fail5: > > > cxgb4_free_atid(ep->com.dev->rdev.lldi.tids, ep->atid); > > > fail2: > > > > Don't like the out-of-order label. Care to rename all of them to make them > > more descriptive and avoid this 4, 3, 5, 2 sequence? > > That seemed incredibly dangerous to me. I'm not sure I could get it right. > How about I introduce this one with a name, like most drivers do? Sure. I was asking if you'd do that for all of them in this func while you were at it.
diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c index b31adec05009..3a0d4ce471c8 100644 --- a/drivers/infiniband/hw/cxgb4/cm.c +++ b/drivers/infiniband/hw/cxgb4/cm.c @@ -558,7 +558,7 @@ static void act_open_req_arp_failure(void *handle, struct sk_buff *skb) cxgb4_clip_release(ep->com.dev->rdev.lldi.ports[0], (const u32 *)&sin6->sin6_addr.s6_addr, 1); } - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, ep->atid); + xa_erase_irq(&ep->com.dev->atids, ep->atid); cxgb4_free_atid(ep->com.dev->rdev.lldi.tids, ep->atid); queue_arp_failure_cpl(ep, skb, FAKE_CPL_PUT_EP_SAFE); } @@ -1201,7 +1201,7 @@ static int act_establish(struct c4iw_dev *dev, struct sk_buff *skb) set_emss(ep, tcp_opt); /* dealloc the atid */ - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, atid); + xa_erase_irq(&ep->com.dev->atids, atid); cxgb4_free_atid(t, atid); set_bit(ACT_ESTAB, &ep->com.history); @@ -2147,7 +2147,9 @@ static int c4iw_reconnect(struct c4iw_ep *ep) err = -ENOMEM; goto fail2; } - insert_handle(ep->com.dev, &ep->com.dev->atid_idr, ep, ep->atid); + err = xa_insert_irq(&ep->com.dev->atids, ep->atid, ep, GFP_KERNEL); + if (err) + goto fail5; /* find a route */ if (ep->com.cm_id->m_local_addr.ss_family == AF_INET) { @@ -2198,7 +2200,8 @@ static int c4iw_reconnect(struct c4iw_ep *ep) fail4: dst_release(ep->dst); fail3: - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, ep->atid); + xa_erase_irq(&ep->com.dev->atids, ep->atid); +fail5: cxgb4_free_atid(ep->com.dev->rdev.lldi.tids, ep->atid); fail2: /* @@ -2281,8 +2284,7 @@ static int act_open_rpl(struct c4iw_dev *dev, struct sk_buff *skb) (const u32 *) &sin6->sin6_addr.s6_addr, 1); } - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, - atid); + xa_erase_irq(&ep->com.dev->atids, atid); cxgb4_free_atid(t, atid); dst_release(ep->dst); cxgb4_l2t_release(ep->l2t); @@ -2319,7 +2321,7 @@ static int act_open_rpl(struct c4iw_dev *dev, struct sk_buff *skb) cxgb4_remove_tid(ep->com.dev->rdev.lldi.tids, 0, GET_TID(rpl), ep->com.local_addr.ss_family); - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, atid); + xa_erase_irq(&ep->com.dev->atids, atid); cxgb4_free_atid(t, atid); dst_release(ep->dst); cxgb4_l2t_release(ep->l2t); @@ -3265,7 +3267,9 @@ int c4iw_connect(struct iw_cm_id *cm_id, struct iw_cm_conn_param *conn_param) err = -ENOMEM; goto fail2; } - insert_handle(dev, &dev->atid_idr, ep, ep->atid); + err = xa_insert_irq(&dev->atids, ep->atid, ep, GFP_KERNEL); + if (err) + goto fail5; memcpy(&ep->com.local_addr, &cm_id->m_local_addr, sizeof(ep->com.local_addr)); @@ -3353,7 +3357,8 @@ int c4iw_connect(struct iw_cm_id *cm_id, struct iw_cm_conn_param *conn_param) fail4: dst_release(ep->dst); fail3: - remove_handle(ep->com.dev, &ep->com.dev->atid_idr, ep->atid); + xa_erase_irq(&ep->com.dev->atids, ep->atid); +fail5: cxgb4_free_atid(ep->com.dev->rdev.lldi.tids, ep->atid); fail2: skb_queue_purge(&ep->com.ep_skb_list); @@ -3687,7 +3692,7 @@ static void active_ofld_conn_reply(struct c4iw_dev *dev, struct sk_buff *skb, cxgb4_clip_release(ep->com.dev->rdev.lldi.ports[0], (const u32 *)&sin6->sin6_addr.s6_addr, 1); } - remove_handle(dev, &dev->atid_idr, atid); + xa_erase_irq(&dev->atids, atid); cxgb4_free_atid(dev->rdev.lldi.tids, atid); dst_release(ep->dst); cxgb4_l2t_release(ep->l2t); diff --git a/drivers/infiniband/hw/cxgb4/device.c b/drivers/infiniband/hw/cxgb4/device.c index 7fae6e68c8f4..0bfba18a12d3 100644 --- a/drivers/infiniband/hw/cxgb4/device.c +++ b/drivers/infiniband/hw/cxgb4/device.c @@ -617,11 +617,6 @@ static int dump_ep(struct c4iw_ep *ep, struct c4iw_debugfs_data *epd) return 0; } -static int _dump_ep(int id, void *p, void *data) -{ - return dump_ep(p, data); -} - static int dump_listen_ep(int id, void *p, void *data) { struct c4iw_listen_ep *ep = p; @@ -695,8 +690,9 @@ static int ep_open(struct inode *inode, struct file *file) xa_for_each(&epd->devp->hwtids, index, ep) count++; + xa_for_each(&epd->devp->atids, index, ep) + count++; spin_lock_irq(&epd->devp->lock); - idr_for_each(&epd->devp->atid_idr, count_idrs, &count); idr_for_each(&epd->devp->stid_idr, count_idrs, &count); spin_unlock_irq(&epd->devp->lock); @@ -711,8 +707,11 @@ static int ep_open(struct inode *inode, struct file *file) xa_for_each(&epd->devp->hwtids, index, ep) dump_ep(ep, epd); xa_unlock_irq(&epd->devp->hwtids); + xa_lock_irq(&epd->devp->atids); + xa_for_each(&epd->devp->atids, index, ep) + dump_ep(ep, epd); + xa_unlock_irq(&epd->devp->atids); spin_lock_irq(&epd->devp->lock); - idr_for_each(&epd->devp->atid_idr, _dump_ep, epd); idr_for_each(&epd->devp->stid_idr, dump_listen_ep, epd); spin_unlock_irq(&epd->devp->lock); @@ -937,7 +936,6 @@ void c4iw_dealloc(struct uld_ctx *ctx) c4iw_rdev_close(&ctx->dev->rdev); wait_event(ctx->dev->wait, xa_empty(&ctx->dev->hwtids)); idr_destroy(&ctx->dev->stid_idr); - idr_destroy(&ctx->dev->atid_idr); if (ctx->dev->rdev.bar2_kva) iounmap(ctx->dev->rdev.bar2_kva); if (ctx->dev->rdev.oc_mw_kva) @@ -1045,8 +1043,8 @@ static struct c4iw_dev *c4iw_alloc(const struct cxgb4_lld_info *infop) xa_init_flags(&devp->qps, XA_FLAGS_LOCK_IRQ); xa_init_flags(&devp->mrs, XA_FLAGS_LOCK_IRQ); xa_init_flags(&devp->hwtids, XA_FLAGS_LOCK_IRQ); + xa_init_flags(&devp->atids, XA_FLAGS_LOCK_IRQ); idr_init(&devp->stid_idr); - idr_init(&devp->atid_idr); spin_lock_init(&devp->lock); mutex_init(&devp->rdev.stats.lock); mutex_init(&devp->db_mutex); diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h index 37003c3b8d51..6ff687f32249 100644 --- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h +++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h @@ -323,7 +323,7 @@ struct c4iw_dev { struct dentry *debugfs_root; enum db_state db_state; struct xarray hwtids; - struct idr atid_idr; + struct xarray atids; struct idr stid_idr; struct list_head db_fc_list; u32 avail_ird;
Signed-off-by: Matthew Wilcox <willy@infradead.org> --- drivers/infiniband/hw/cxgb4/cm.c | 25 +++++++++++++++---------- drivers/infiniband/hw/cxgb4/device.c | 16 +++++++--------- drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 2 +- 3 files changed, 23 insertions(+), 20 deletions(-)