Re: [PATCH v2 3/5] tasklet: Remove the old-softirq implementation.

["Jan Beulich" <JBeulich@suse.com>]

>>> On 25.08.16 at 21:23, <konrad.wilk@oracle.com> wrote:
> With the new percpu tasklet (see "tasklet: Introduce per-cpu tasklet."
> and "tasklet: Add cross CPU feeding of per-cpu-tasklets") we have
> now in a place a working version of per-cpu softirq tasklets.
> 
> We can now remove the old implementation of the
> softirq tasklet. We also remove the temporary scaffolding
> of TASKLET_SOFTIRQ_PERCPU. Further removal of code will
> be done in "tasklet: Remove the old scaffolding" once the
> schedule tasklet code is in.
> 
> This could be squashed in "tasklet: Introduce per-cpu
> tasklet for softirq." but the author thought it would
> be an easier aid in understanding the code with these
> parts split out.

When you said something in the previous patch I tended to agree.
However, seeing this I'm not sure the split up isn't ending up worse
to look at than if you had switched over in one go, retaining e.g. ...

> --- a/xen/common/tasklet.c
> +++ b/xen/common/tasklet.c
> @@ -26,7 +26,6 @@ static bool_t tasklets_initialised;
>  DEFINE_PER_CPU(unsigned long, tasklet_work_to_do);
>  
>  static DEFINE_PER_CPU(struct list_head, tasklet_list);
> -static DEFINE_PER_CPU(struct list_head, softirq_tasklet_list);

... this original list name (I now can guess why in patch 1 you named
the new list the way you did, but for the end result I think this old
name is to be preferred, as what you add is not a list of softirqs).

> @@ -89,18 +88,14 @@ static void tasklet_enqueue(struct tasklet *t)
>  
>          list = &__get_cpu_var(softirq_list);
>          list_add_tail(&t->list, list);
> -        raise_softirq(TASKLET_SOFTIRQ_PERCPU);
> +        raise_softirq(TASKLET_SOFTIRQ);
>  
>          local_irq_restore(flags);
>          return;
>      }
>      if ( t->is_softirq )
>      {
> -        struct list_head *list = &per_cpu(softirq_tasklet_list, cpu);
> -        bool_t was_empty = list_empty(list);
> -        list_add_tail(&t->list, list);
> -        if ( was_empty )
> -            cpu_raise_softirq(cpu, TASKLET_SOFTIRQ);
> +        BUG();

I don't think you need the is_softirq flag anymore at this point,
which would eliminate this if() altogether.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel