[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 04/10] xen/blkfront: split per device io_lock
On Mon, Nov 02, 2015 at 12:21:40PM +0800, Bob Liu wrote: > The per device io_lock became a coarser grained lock after multi-queues/rings > was introduced, this patch introduced a fine-grained ring_lock for each ring. s/was introduced/was introduced (see commit titled XYZ)/ s/introdued/introduces/ > > The old io_lock was renamed to dev_lock and only protect the ->grants list s/was/is/ s/protect/protects/ > which is shared by all rings. > > Signed-off-by: Bob Liu <bob.liu@xxxxxxxxxx> > --- > drivers/block/xen-blkfront.c | 57 > ++++++++++++++++++++++++++------------------ > 1 file changed, 34 insertions(+), 23 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index eab78e7..8cc5995 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -121,6 +121,7 @@ MODULE_PARM_DESC(max_ring_page_order, "Maximum order of > pages to be used for the > */ > struct blkfront_ring_info { > struct blkif_front_ring ring; Can you add a comment explaining the lock semantic? As in under what conditions should it be taken? Like you have it below. > + spinlock_t ring_lock; > unsigned int ring_ref[XENBUS_MAX_RING_PAGES]; > unsigned int evtchn, irq; > struct work_struct work; > @@ -138,7 +139,8 @@ struct blkfront_ring_info { > */ > struct blkfront_info > { > - spinlock_t io_lock; > + /* Lock to proect info->grants list shared by multi rings */ s/proect/protect/ Missing full stop. > + spinlock_t dev_lock; Shouldn't it be right next to what it is protecting? That is right below (or above): 'struct list_head grants;'? > struct mutex mutex; > struct xenbus_device *xbdev; > struct gendisk *gd; > @@ -224,6 +226,7 @@ static int fill_grant_buffer(struct blkfront_ring_info > *rinfo, int num) > struct grant *gnt_list_entry, *n; > int i = 0; > > + spin_lock_irq(&info->dev_lock); Why there? Why not where you add it to the list? > while(i < num) { > gnt_list_entry = kzalloc(sizeof(struct grant), GFP_NOIO); > if (!gnt_list_entry) > @@ -242,6 +245,7 @@ static int fill_grant_buffer(struct blkfront_ring_info > *rinfo, int num) > list_add(&gnt_list_entry->node, &info->grants); Right here that is? You are holding the lock for the duration of 'kzalloc' and 'alloc_page'. And more interestingly, GFP_NOIO translates to __GFP_WAIT which means it can call 'schedule'. - And you have taken an spinlock. That should have thrown lots of warnings? > i++; > } > + spin_unlock_irq(&info->dev_lock); > > return 0; > > @@ -254,6 +258,7 @@ out_of_memory: > kfree(gnt_list_entry); > i--; > } > + spin_unlock_irq(&info->dev_lock); Just do it around the 'list_del' operation. You are using an 'safe' > BUG_ON(i != 0); > return -ENOMEM; > } > @@ -265,6 +270,7 @@ static struct grant *get_grant(grant_ref_t *gref_head, > struct grant *gnt_list_entry; > unsigned long buffer_gfn; > > + spin_lock(&info->dev_lock); > BUG_ON(list_empty(&info->grants)); > gnt_list_entry = list_first_entry(&info->grants, struct grant, > node); > @@ -272,8 +278,10 @@ static struct grant *get_grant(grant_ref_t *gref_head, > > if (gnt_list_entry->gref != GRANT_INVALID_REF) { > info->persistent_gnts_c--; > + spin_unlock(&info->dev_lock); > return gnt_list_entry; > } > + spin_unlock(&info->dev_lock); Just have one spin_unlock. Put it right before the 'if (gnt_list_entry->gref)..'. > > /* Assign a gref to this page */ > gnt_list_entry->gref = gnttab_claim_grant_reference(gref_head); > @@ -639,7 +647,7 @@ static int blkif_queue_rq(struct blk_mq_hw_ctx *hctx, > struct blkfront_ring_info *rinfo = (struct blkfront_ring_info > *)hctx->driver_data; > > blk_mq_start_request(qd->rq); > - spin_lock_irq(&info->io_lock); > + spin_lock_irq(&rinfo->ring_lock); > if (RING_FULL(&rinfo->ring)) > goto out_busy; > > @@ -650,15 +658,15 @@ static int blkif_queue_rq(struct blk_mq_hw_ctx *hctx, > goto out_busy; > > flush_requests(rinfo); > - spin_unlock_irq(&info->io_lock); > + spin_unlock_irq(&rinfo->ring_lock); > return BLK_MQ_RQ_QUEUE_OK; > > out_err: > - spin_unlock_irq(&info->io_lock); > + spin_unlock_irq(&rinfo->ring_lock); > return BLK_MQ_RQ_QUEUE_ERROR; > > out_busy: > - spin_unlock_irq(&info->io_lock); > + spin_unlock_irq(&rinfo->ring_lock); > blk_mq_stop_hw_queue(hctx); > return BLK_MQ_RQ_QUEUE_BUSY; > } > @@ -959,21 +967,22 @@ static void xlvbd_release_gendisk(struct blkfront_info > *info) > info->gd = NULL; > } > > -/* Must be called with io_lock holded */ > static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) > { > + unsigned long flags; > + > + spin_lock_irqsave(&rinfo->ring_lock, flags); > if (!RING_FULL(&rinfo->ring)) > blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true); > + spin_unlock_irqrestore(&rinfo->ring_lock, flags); > } > > static void blkif_restart_queue(struct work_struct *work) > { > struct blkfront_ring_info *rinfo = container_of(work, struct > blkfront_ring_info, work); > > - spin_lock_irq(&rinfo->dev_info->io_lock); > if (rinfo->dev_info->connected == BLKIF_STATE_CONNECTED) > kick_pending_request_queues(rinfo); > - spin_unlock_irq(&rinfo->dev_info->io_lock); > } > > static void blkif_free_ring(struct blkfront_ring_info *rinfo) > @@ -1065,7 +1074,7 @@ static void blkif_free(struct blkfront_info *info, int > suspend) > int i; > > /* Prevent new requests being issued until we fix things up. */ > - spin_lock_irq(&info->io_lock); > + spin_lock_irq(&info->dev_lock); I would move that lock to be right before: if (!list_empty(&info->grants)). > info->connected = suspend ? > BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED; > /* No more blkif_request(). */ > @@ -1091,7 +1100,7 @@ static void blkif_free(struct blkfront_info *info, int > suspend) > > for (i = 0; i < info->nr_rings; i++) > blkif_free_ring(&info->rinfo[i]); > - spin_unlock_irq(&info->io_lock); > + spin_unlock_irq(&info->dev_lock); Please move them before this loop. The blkif_free_ring is not touching the 'grants' list. > } > > static void blkif_completion(struct blk_shadow *s, struct blkfront_ring_info > *rinfo, > @@ -1103,6 +1112,7 @@ static void blkif_completion(struct blk_shadow *s, > struct blkfront_ring_info *ri > void *shared_data; > int nseg; > struct blkfront_info *info = rinfo->dev_info; > + unsigned long flags; > > nseg = s->req.operation == BLKIF_OP_INDIRECT ? > s->req.u.indirect.nr_segments : s->req.u.rw.nr_segments; > @@ -1120,6 +1130,7 @@ static void blkif_completion(struct blk_shadow *s, > struct blkfront_ring_info *ri > kunmap_atomic(shared_data); > } > } > + ??? Stray? > /* Add the persistent grant into the list of free grants */ > for (i = 0; i < nseg; i++) { > if (gnttab_query_foreign_access(s->grants_used[i]->gref)) { > @@ -1132,8 +1143,10 @@ static void blkif_completion(struct blk_shadow *s, > struct blkfront_ring_info *ri > if (!info->feature_persistent) > pr_alert_ratelimited("backed has not unmapped > grant: %u\n", > s->grants_used[i]->gref); > + spin_lock_irqsave(&info->dev_lock, flags); > list_add(&s->grants_used[i]->node, &info->grants); > info->persistent_gnts_c++; > + spin_unlock_irqrestore(&info->dev_lock, flags); Are we also protecting the persistent_gnts_c value? If so that should be documented in the 'struct blkfront_info' > } else { > /* > * If the grant is not mapped by the backend we end the > @@ -1143,7 +1156,9 @@ static void blkif_completion(struct blk_shadow *s, > struct blkfront_ring_info *ri > */ > gnttab_end_foreign_access(s->grants_used[i]->gref, 0, > 0UL); > s->grants_used[i]->gref = GRANT_INVALID_REF; > + spin_lock_irqsave(&info->dev_lock, flags); > list_add_tail(&s->grants_used[i]->node, &info->grants); > + spin_unlock_irqrestore(&info->dev_lock, flags); > } > } > if (s->req.operation == BLKIF_OP_INDIRECT) { > @@ -1152,8 +1167,10 @@ static void blkif_completion(struct blk_shadow *s, > struct blkfront_ring_info *ri > if (!info->feature_persistent) > pr_alert_ratelimited("backed has not > unmapped grant: %u\n", > > s->indirect_grants[i]->gref); > + spin_lock_irqsave(&info->dev_lock, flags); > list_add(&s->indirect_grants[i]->node, > &info->grants); > info->persistent_gnts_c++; > + spin_unlock_irqrestore(&info->dev_lock, flags); > } else { > struct page *indirect_page; > > @@ -1162,12 +1179,14 @@ static void blkif_completion(struct blk_shadow *s, > struct blkfront_ring_info *ri > * Add the used indirect page back to the list > of > * available pages for indirect grefs. > */ > + spin_lock_irqsave(&info->dev_lock, flags); > if (!info->feature_persistent) { > indirect_page = > pfn_to_page(s->indirect_grants[i]->pfn); > list_add(&indirect_page->lru, > &rinfo->indirect_pages); > } > s->indirect_grants[i]->gref = GRANT_INVALID_REF; > list_add_tail(&s->indirect_grants[i]->node, > &info->grants); > + spin_unlock_irqrestore(&info->dev_lock, flags); > } > } > } > @@ -1183,13 +1202,10 @@ static irqreturn_t blkif_interrupt(int irq, void > *dev_id) > struct blkfront_info *info = rinfo->dev_info; > int error; > > - spin_lock_irqsave(&info->io_lock, flags); > - > - if (unlikely(info->connected != BLKIF_STATE_CONNECTED)) { > - spin_unlock_irqrestore(&info->io_lock, flags); > + if (unlikely(info->connected != BLKIF_STATE_CONNECTED)) > return IRQ_HANDLED; > - } > > + spin_lock_irqsave(&rinfo->ring_lock, flags); > again: > rp = rinfo->ring.sring->rsp_prod; > rmb(); /* Ensure we see queued responses up to 'rp'. */ > @@ -1280,10 +1296,9 @@ static irqreturn_t blkif_interrupt(int irq, void > *dev_id) > } else > rinfo->ring.sring->rsp_event = i + 1; > > + spin_unlock_irqrestore(&rinfo->ring_lock, flags); > kick_pending_request_queues(rinfo); We drop the lock and then take it again in 'kick_pending_request_queues' Would it be better if we have an argument 'locked' which would bypass the taking of the lock once more? > > - spin_unlock_irqrestore(&info->io_lock, flags); > - > return IRQ_HANDLED; > } > > @@ -1529,10 +1544,11 @@ static int blkfront_probe(struct xenbus_device *dev, > INIT_LIST_HEAD(&rinfo->indirect_pages); > rinfo->dev_info = info; > INIT_WORK(&rinfo->work, blkif_restart_queue); > + spin_lock_init(&rinfo->ring_lock); > } > > mutex_init(&info->mutex); > - spin_lock_init(&info->io_lock); > + spin_lock_init(&info->dev_lock); > info->xbdev = dev; > info->vdevice = vdevice; > INIT_LIST_HEAD(&info->grants); > @@ -1629,8 +1645,6 @@ static int blkif_recover(struct blkfront_info *info) > } > xenbus_switch_state(info->xbdev, XenbusStateConnected); > > - spin_lock_irq(&info->io_lock); > - > /* Now safe for us to use the shared ring */ > info->connected = BLKIF_STATE_CONNECTED; > > @@ -1648,7 +1662,6 @@ static int blkif_recover(struct blkfront_info *info) > BUG_ON(req->nr_phys_segments > segs); > blk_mq_requeue_request(req); > } > - spin_unlock_irq(&info->io_lock); > blk_mq_kick_requeue_list(info->rq); > > while ((bio = bio_list_pop(&bio_list)) != NULL) { > @@ -1994,11 +2007,9 @@ static void blkfront_connect(struct blkfront_info > *info) > xenbus_switch_state(info->xbdev, XenbusStateConnected); > > /* Kick pending requests. */ > - spin_lock_irq(&info->io_lock); > info->connected = BLKIF_STATE_CONNECTED; > for (i = 0; i < info->nr_rings; i++) > kick_pending_request_queues(&info->rinfo[i]); > - spin_unlock_irq(&info->io_lock); > > add_disk(info->gd); > > -- > 1.8.3.1 > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |