lkml.org 
[lkml]   [2016]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 1/2] lib/percpu-list: Per-cpu list with associated per-cpu locks
On 02/17/2016 04:53 AM, Dave Chinner wrote:
> On Tue, Feb 16, 2016 at 08:31:19PM -0500, Waiman Long wrote:
>> Linked list is used everywhere in the Linux kernel. However, if many
>> threads are trying to add or delete entries into the same linked list,
>> it can create a performance bottleneck.
>>
>> This patch introduces a new per-cpu list subystem with associated
>> per-cpu locks for protecting each of the lists individually. This
>> allows list entries insertion and deletion operations to happen in
>> parallel instead of being serialized with a global list and lock.
>>
>> List entry insertion is strictly per cpu. List deletion, however, can
>> happen in a cpu other than the one that did the insertion. So we still
>> need lock to protect the list. Because of that, there may still be
>> a small amount of contention when deletion is being done.
>>
>> A new header file include/linux/percpu-list.h will be added with the
>> associated percpu_list structure. The following functions are used
>> to manage the per-cpu list:
>>
>> 1. int init_percpu_list_head(struct percpu_list **pclist_handle)
>> 2. void percpu_list_add(struct percpu_list *new,
>> struct percpu_list *head)
>> 3. void percpu_list_del(struct percpu_list *entry)
> A few comments on the code
>
>> + * A per-cpu list protected by a per-cpu spinlock.
>> + *
>> + * The list head percpu_list structure contains the spinlock, the other
>> + * entries in the list contain the spinlock pointer.
>> + */
>> +struct percpu_list {
>> + struct list_head list;
>> + union {
>> + spinlock_t lock; /* For list head */
>> + spinlock_t *lockptr; /* For other entries */
>> + };
>> +};
> This union is bad for kernels running spinlock debugging - the size
> of the spinlock can blow out, and that increases the size of any
> object that has a percpu_list in it. I've only got basic spinlock
> debugging turned on, and the spinlock_t is 24 bytes with that
> config. If I turn on lockdep, it gets much larger again....
>
> So it might be best to separate the list head and list entry
> structures, similar to a hash list?

Right. I will split it into 2 separate structure in the next iteration
of the patch.

>> +static inline void INIT_PERCPU_LIST_HEAD(struct percpu_list *pcpu_list)
>> +{
>> + INIT_LIST_HEAD(&pcpu_list->list);
>> + pcpu_list->lock = __SPIN_LOCK_UNLOCKED(&pcpu_list->lock);
>> +}
>> +
>> +static inline void INIT_PERCPU_LIST_ENTRY(struct percpu_list *pcpu_list)
>> +{
>> + INIT_LIST_HEAD(&pcpu_list->list);
>> + pcpu_list->lockptr = NULL;
>> +}
> These function names don't need to shout.

I was just following the convention used in list init functions. I can
certainly change them to lowercase.

>
>> +/**
>> + * for_all_percpu_list_entries - iterate over all the per-cpu list with locking
>> + * @pos: the type * to use as a loop cursor for the current .
>> + * @next: an internal type * variable pointing to the next entry
>> + * @pchead: an internal struct list * of percpu list head
>> + * @pclock: an internal variable for the current per-cpu spinlock
>> + * @head: the head of the per-cpu list
>> + * @member: the name of the per-cpu list within the struct
>> + */
>> +#define for_all_percpu_list_entries(pos, next, pchead, pclock, head, member)\
>> + { \
>> + int cpu; \
>> + for_each_possible_cpu (cpu) { \
>> + typeof(*pos) *next; \
>> + spinlock_t *pclock = per_cpu_ptr(&(head)->lock, cpu); \
>> + struct list_head *pchead =&per_cpu_ptr(head, cpu)->list;\
>> + spin_lock(pclock); \
>> + list_for_each_entry_safe(pos, next, pchead, member.list)
>> +
>> +#define end_all_percpu_list_entries(pclock) spin_unlock(pclock); } }
> This is a bit of a landmine - the code inside he iteration is under
> a spinlock hidden in the macros. People are going to forget about
> that, and it's needs documenting about how it needs to be treated
> w.r.t. dropping and regaining the lock so sleeping operations can be
> performed on objects on the list being iterated.
>
> Can we also think up some shorter names - names that are 30-40
> characters long are getting out out of hand given we're supposed
> tobe sticking to 80 character widths and we lost 8 of them in the
> first indent...
>
> Cheers,
>
> Dave.

I will try to shorten the name and better document the macro. This is
probably the most tricky part of the whole part.

Cheers,
Longman

\
 
 \ /
  Last update: 2016-02-17 17:21    [W:0.106 / U:0.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site