Hi Jan,
I'm currently working on some cleanups on IPI code too and working on top
of these patches, just have a few comments:
On Mon, Dec 23, 2013 at 09:39:23PM +0100, Jan Kara wrote:
> Abusing rq->csd.list for a list of requests to complete is rather ugly.
> Especially since using queuelist should be safe and much cleaner.
It would be nice to have a few more details that explain why doing so is safe
wrt a block request lifecycle. At least something that tells why rq->queuelist
can't be ever used concurrently by the time we send the IPI and we trigger/raise
the softirq.
>
> Signed-off-by: Jan Kara <[email protected]>
> ---
> block/blk-softirq.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-softirq.c b/block/blk-softirq.c
> index 57790c1a97eb..7ea5534096d5 100644
> --- a/block/blk-softirq.c
> +++ b/block/blk-softirq.c
> @@ -30,8 +30,8 @@ static void blk_done_softirq(struct softirq_action *h)
> while (!list_empty(&local_list)) {
> struct request *rq;
>
> - rq = list_entry(local_list.next, struct request, csd.list);
> - list_del_init(&rq->csd.list);
> + rq = list_entry(local_list.next, struct request, queuelist);
> + list_del_init(&rq->queuelist);
> rq->q->softirq_done_fn(rq);
> }
> }
> @@ -45,9 +45,9 @@ static void trigger_softirq(void *data)
>
> local_irq_save(flags);
> list = this_cpu_ptr(&blk_cpu_done);
> - list_add_tail(&rq->csd.list, list);
> + list_add_tail(&rq->queuelist, list);
And given that's an alternate use of rq->queuelist, perhaps add a comment
to unconfuse people.
Thanks.
>
> - if (list->next == &rq->csd.list)
> + if (list->next == &rq->queuelist)
> raise_softirq_irqoff(BLOCK_SOFTIRQ);
>
> local_irq_restore(flags);
> @@ -136,7 +136,7 @@ void __blk_complete_request(struct request *req)
> struct list_head *list;
> do_local:
> list = this_cpu_ptr(&blk_cpu_done);
> - list_add_tail(&req->csd.list, list);
> + list_add_tail(&req->queuelist, list);
>
> /*
> * if the list only contains our just added request,
> @@ -144,7 +144,7 @@ do_local:
> * entries there, someone already raised the irq but it
> * hasn't run yet.
> */
> - if (list->next == &req->csd.list)
> + if (list->next == &req->queuelist)
> raise_softirq_irqoff(BLOCK_SOFTIRQ);
> } else if (raise_blk_irq(ccpu, req))
> goto do_local;
> --
> 1.8.1.4
>
Hi,
On Thu 30-01-14 13:39:18, Frederic Weisbecker wrote:
> I'm currently working on some cleanups on IPI code too and working on top
> of these patches, just have a few comments:
Great, thanks!
> On Mon, Dec 23, 2013 at 09:39:23PM +0100, Jan Kara wrote:
> > Abusing rq->csd.list for a list of requests to complete is rather ugly.
> > Especially since using queuelist should be safe and much cleaner.
>
> It would be nice to have a few more details that explain why doing so is safe
> wrt a block request lifecycle. At least something that tells why rq->queuelist
> can't be ever used concurrently by the time we send the IPI and we trigger/raise
> the softirq.
Sure. Should I send the patch to you with an updated changelog and added
comment you requested?
> > Signed-off-by: Jan Kara <[email protected]>
> > ---
> > block/blk-softirq.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/block/blk-softirq.c b/block/blk-softirq.c
> > index 57790c1a97eb..7ea5534096d5 100644
> > --- a/block/blk-softirq.c
> > +++ b/block/blk-softirq.c
> > @@ -30,8 +30,8 @@ static void blk_done_softirq(struct softirq_action *h)
> > while (!list_empty(&local_list)) {
> > struct request *rq;
> >
> > - rq = list_entry(local_list.next, struct request, csd.list);
> > - list_del_init(&rq->csd.list);
> > + rq = list_entry(local_list.next, struct request, queuelist);
> > + list_del_init(&rq->queuelist);
> > rq->q->softirq_done_fn(rq);
> > }
> > }
> > @@ -45,9 +45,9 @@ static void trigger_softirq(void *data)
> >
> > local_irq_save(flags);
> > list = this_cpu_ptr(&blk_cpu_done);
> > - list_add_tail(&rq->csd.list, list);
> > + list_add_tail(&rq->queuelist, list);
>
> And given that's an alternate use of rq->queuelist, perhaps add a comment
> to unconfuse people.
Good idea, will do.
Honza
> >
> > - if (list->next == &rq->csd.list)
> > + if (list->next == &rq->queuelist)
> > raise_softirq_irqoff(BLOCK_SOFTIRQ);
> >
> > local_irq_restore(flags);
> > @@ -136,7 +136,7 @@ void __blk_complete_request(struct request *req)
> > struct list_head *list;
> > do_local:
> > list = this_cpu_ptr(&blk_cpu_done);
> > - list_add_tail(&req->csd.list, list);
> > + list_add_tail(&req->queuelist, list);
> >
> > /*
> > * if the list only contains our just added request,
> > @@ -144,7 +144,7 @@ do_local:
> > * entries there, someone already raised the irq but it
> > * hasn't run yet.
> > */
> > - if (list->next == &req->csd.list)
> > + if (list->next == &req->queuelist)
> > raise_softirq_irqoff(BLOCK_SOFTIRQ);
> > } else if (raise_blk_irq(ccpu, req))
> > goto do_local;
> > --
> > 1.8.1.4
> >
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, Jan 30, 2014 at 04:45:23PM +0100, Jan Kara wrote:
> Hi,
>
> On Thu 30-01-14 13:39:18, Frederic Weisbecker wrote:
> > I'm currently working on some cleanups on IPI code too and working on top
> > of these patches, just have a few comments:
> Great, thanks!
>
> > On Mon, Dec 23, 2013 at 09:39:23PM +0100, Jan Kara wrote:
> > > Abusing rq->csd.list for a list of requests to complete is rather ugly.
> > > Especially since using queuelist should be safe and much cleaner.
> >
> > It would be nice to have a few more details that explain why doing so is safe
> > wrt a block request lifecycle. At least something that tells why rq->queuelist
> > can't be ever used concurrently by the time we send the IPI and we trigger/raise
> > the softirq.
> Sure. Should I send the patch to you with an updated changelog and added
> comment you requested?
Yeah that would be nice!
For more precision, I would like to apply these:
1) block: Stop abusing csd.list for fifo_time
2) block: Stop abusing rq->csd.list in blk-softirq
3) kernel: use lockless list for smp_call_function_single()
4) smp: Teach __smp_call_function_single() to check for offline cpus
This is because I need to tweak a bit the IPI code for some nohz
functionnality. I'm not sure about the result but in any case these
llist based cleanups look very nice, so lets try to push them. I'm also
working on some consolidation between __smp_call_function_single()
and smp_call_function_single() since they share almost the same code.
Also this shouldn't conflict with Andrew's tree if he applies these as well
since -mm is based on -next. And the printk part should still go through his
tree I think.
>
> > > Signed-off-by: Jan Kara <[email protected]>
> > > ---
> > > block/blk-softirq.c | 12 ++++++------
> > > 1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/block/blk-softirq.c b/block/blk-softirq.c
> > > index 57790c1a97eb..7ea5534096d5 100644
> > > --- a/block/blk-softirq.c
> > > +++ b/block/blk-softirq.c
> > > @@ -30,8 +30,8 @@ static void blk_done_softirq(struct softirq_action *h)
> > > while (!list_empty(&local_list)) {
> > > struct request *rq;
> > >
> > > - rq = list_entry(local_list.next, struct request, csd.list);
> > > - list_del_init(&rq->csd.list);
> > > + rq = list_entry(local_list.next, struct request, queuelist);
> > > + list_del_init(&rq->queuelist);
> > > rq->q->softirq_done_fn(rq);
> > > }
> > > }
> > > @@ -45,9 +45,9 @@ static void trigger_softirq(void *data)
> > >
> > > local_irq_save(flags);
> > > list = this_cpu_ptr(&blk_cpu_done);
> > > - list_add_tail(&rq->csd.list, list);
> > > + list_add_tail(&rq->queuelist, list);
> >
> > And given that's an alternate use of rq->queuelist, perhaps add a comment
> > to unconfuse people.
> Good idea, will do.
Thanks!
On Thu 30-01-14 18:01:20, Frederic Weisbecker wrote:
> On Thu, Jan 30, 2014 at 04:45:23PM +0100, Jan Kara wrote:
> > Hi,
> >
> > On Thu 30-01-14 13:39:18, Frederic Weisbecker wrote:
> > > I'm currently working on some cleanups on IPI code too and working on top
> > > of these patches, just have a few comments:
> > Great, thanks!
> >
> > > On Mon, Dec 23, 2013 at 09:39:23PM +0100, Jan Kara wrote:
> > > > Abusing rq->csd.list for a list of requests to complete is rather ugly.
> > > > Especially since using queuelist should be safe and much cleaner.
> > >
> > > It would be nice to have a few more details that explain why doing so is safe
> > > wrt a block request lifecycle. At least something that tells why rq->queuelist
> > > can't be ever used concurrently by the time we send the IPI and we trigger/raise
> > > the softirq.
> > Sure. Should I send the patch to you with an updated changelog and added
> > comment you requested?
>
> Yeah that would be nice!
OK, the updated patch is attached.
> For more precision, I would like to apply these:
>
> 1) block: Stop abusing csd.list for fifo_time
> 2) block: Stop abusing rq->csd.list in blk-softirq
> 3) kernel: use lockless list for smp_call_function_single()
> 4) smp: Teach __smp_call_function_single() to check for offline cpus
>
> This is because I need to tweak a bit the IPI code for some nohz
> functionnality. I'm not sure about the result but in any case these
> llist based cleanups look very nice, so lets try to push them. I'm also
> working on some consolidation between __smp_call_function_single()
> and smp_call_function_single() since they share almost the same code.
>
> Also this shouldn't conflict with Andrew's tree if he applies these as well
> since -mm is based on -next. And the printk part should still go through his
> tree I think.
Sure, that should be no problem. Jens might have the patch somewhere in
the linux-block.git but any conflict should be easy to resolve.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, Jan 30, 2014 at 11:12:41PM +0100, Jan Kara wrote:
> On Thu 30-01-14 18:01:20, Frederic Weisbecker wrote:
> > On Thu, Jan 30, 2014 at 04:45:23PM +0100, Jan Kara wrote:
> > > Hi,
> > >
> > > On Thu 30-01-14 13:39:18, Frederic Weisbecker wrote:
> > > > I'm currently working on some cleanups on IPI code too and working on top
> > > > of these patches, just have a few comments:
> > > Great, thanks!
> > >
> > > > On Mon, Dec 23, 2013 at 09:39:23PM +0100, Jan Kara wrote:
> > > > > Abusing rq->csd.list for a list of requests to complete is rather ugly.
> > > > > Especially since using queuelist should be safe and much cleaner.
> > > >
> > > > It would be nice to have a few more details that explain why doing so is safe
> > > > wrt a block request lifecycle. At least something that tells why rq->queuelist
> > > > can't be ever used concurrently by the time we send the IPI and we trigger/raise
> > > > the softirq.
> > > Sure. Should I send the patch to you with an updated changelog and added
> > > comment you requested?
> >
> > Yeah that would be nice!
> OK, the updated patch is attached.
Applied, thanks!
Note that the llist use in smp.c patch from Christoph has been merged
upstream today. But it keeps list_head in a union so I applied your changes
that:
1) remove list_head from smp.c
2) use llist_for_each_entry_safe()
in seperate delta patches. Anyway, I'll send the series soonish.