Refactoring and significantly reducing code complexity

This is a refactored version with the following main changes:

- The parallel workqueue no longer uses the WQ_UNBOUND attribute
- Removal of CPU-related logic, sysfs-related interfaces
- removal of structures like padata_cpumask, and deletion of parallel_data
- Using completion to maintain sequencing
- no longer using lists
- removing structures like padata_list and padata_serial_queue
- Removal of padata_do_serial()

Quoted 9 lines · Wang Jinchao

This is a refactored version with the following main changes:

- The parallel workqueue no longer uses the WQ_UNBOUND attribute
- Removal of CPU-related logic, sysfs-related interfaces
- removal of structures like padata_cpumask, and deletion of parallel_data
- Using completion to maintain sequencing
- no longer using lists
- removing structures like padata_list and padata_serial_queue
- Removal of padata_do_serial()

This removes all the logic that is needed to ensure that the parallelized objects return in the same order as they were before the parallelization. This change makes padata unusable for networking.

Quoted 11 lines · Steffen Klassert

Quoted 9 lines · Wang Jinchao

This is a refactored version with the following main changes:

- The parallel workqueue no longer uses the WQ_UNBOUND attribute
- Removal of CPU-related logic, sysfs-related interfaces
- removal of structures like padata_cpumask, and deletion of parallel_data
- Using completion to maintain sequencing
- no longer using lists
- removing structures like padata_list and padata_serial_queue
- Removal of padata_do_serial()

Quoted 4 lines

This removes all the logic that is needed to ensure that the parallelized objects return in the same order as they were before the parallelization. This change makes padata unusable for networking.

The RFC use the following three to ensure serial timing sequence:

1. Use alloc_ordered_workqueue() to create a serial worker queue where serial() function runs. This ensures that serial() function executes as serial work was enqueued using queue_work().
2. Queue the serial work before enqueueing parallel work in padata_do_parallel(). This ensures the serial work follows the same order as the padata_do_parallel().
3. The serial work wait for completion of parallel_done, which will be complete()ed after the parallel() function within the parallel work.

This is just a design idea, because I am not familiar with IPsec, I haven't tested it in a real network environment yet.
Could you give me some clues on how to use pcrypt in an IPsec scenario?

This is just a design idea, because I am not familiar with IPsec, I haven't tested it in a real network environment yet.

Quoted 2 lines · Wang Jinchao

This is just a design idea, because I am not familiar with IPsec, I haven't tested it in a real network environment yet.

Out of curiosity, what's your use case for padata?

Quoted 9 lines · Wang Jinchao

This is a refactored version with the following main changes:

- The parallel workqueue no longer uses the WQ_UNBOUND attribute
- Removal of CPU-related logic, sysfs-related interfaces
- removal of structures like padata_cpumask, and deletion of parallel_data
- Using completion to maintain sequencing
- no longer using lists
- removing structures like padata_list and padata_serial_queue
- Removal of padata_do_serial()

This removes all the logic that is needed to ensure that the parallelized objects return in the same order as they were before the parallelization. This change makes padata unusable for networking.

This is a refactored version with the following main changes:

- The parallel workqueue no longer uses the WQ_UNBOUND attribute
- Removal of CPU-related logic, sysfs-related interfaces
- removal of structures like padata_cpumask, and deletion of parallel_data
- Using completion to maintain sequencing
- no longer using lists
- removing structures like padata_list and padata_serial_queue
- Removal of padata_do_serial()
- padata automatically ensures the calling sequence.

Testing was conducted using ltp's pcrypt_aead01, and the execution time comparison between the old and new versions is as follows:

Old Version:
real 0m27.451s
user 0m0.031s
sys 0m0.260s

New Version:
real 0m21.351s
user 0m0.023s
sys 0m0.260s

Signed-off-by Wang Jinchao <[email protected]>

---
 crypto/pcrypt.c        |  34 +-
 include/linux/padata.h |  95 +----
 kernel/padata.c        | 815 ++---------------------------------------
 3 files changed, 43 insertions(+), 901 deletions(-)

This is a refactored version with the following main changes:

- The parallel workqueue no longer uses the WQ_UNBOUND attribute

- Removal of CPU-related logic, sysfs-related interfaces

Testing was conducted using ltp's pcrypt_aead01, and the execution time comparison between the old and new versions is as follows:

Old Version:
real 0m27.451s
user 0m0.031s
sys 0m0.260s

New Version:
real 0m21.351s
user 0m0.023s
sys 0m0.260s

(quoted diff) +0 -0

@@ -986,57 +281,27 @@ struct padata_instance *padata_alloc(const char *name)

Hello,

Quoted 1 line · Wang Jinchao

This is a refactored version with the following main changes:

The RFC overall is a nice simplification, and the basic approach of using an ordered workqueue seems to work.

Quoted 1 line

- The parallel workqueue no longer uses the WQ_UNBOUND attribute

What's the justification here? If it improves performance, please show numbers. Earlier tests[0] showed a large improvement when adding this flag.

[0] https://lore.kernel.org/linux-crypto/[email protected]/

Quoted 1 line

- Removal of CPU-related logic, sysfs-related interfaces

I agree with Steffen that we should continue to honor the cpumasks that the user sets.

The simplest way I see to make the parallel mask work with your refactor is to just make the parallel workqueue unbound again, since setting workqueue attributes is only allowed for unbound, and bring back some of the plumbing that leads to the apply_workqueue_attrs call.

The serial mask is trickier. Changing attributes of an ordered workqueue (the cpumask in this case) makes the kernel throw a warning...

static int apply_workqueue_attrs_locked

        ...
        /* creating multiple pwqs breaks ordering guarantee */                   
        if (!list_empty(&wq->pwqs)) {                                            
                if (WARN_ON(wq->flags & __WQ_ORDERED_EXPLICIT))                  
                        return -EINVAL;                                          
                                                                                 
                wq->flags &= ~__WQ_ORDERED;                                      
        }

...but I'm not sure this is a fundamental limitation. The changelog of 0a94efb5acbb ("workqueue: implicit ordered attribute should be overridable") says changes to "max_active and some attribute changes" are rejected, but it might be possible to relax the warning to allow setting a cpumask while still rejecting other changes.

Quoted 9 lines

Testing was conducted using ltp's pcrypt_aead01, and the execution time comparison between the old and new versions is as follows:

Old Version:
real 0m27.451s
user 0m0.031s
sys 0m0.260s

Quoted 4 lines

New Version:
real 0m21.351s
user 0m0.023s
sys 0m0.260s

Great speedup. A test that runs many requests for a long time in parallel is also good to run, such as [0].

Quoted 1 line

(quoted diff) +0 -0

@@ -986,57 +281,27 @@ struct padata_instance *padata_alloc(const char *name)

...

kernel/padata.c struct padata_instance *padata_alloc(const char *name) · line 292

Quoted 3 lines

kernel/padata.c +3 -0

+	pinst->serial_wq = alloc_ordered_workqueue ("%s_serial",
+									WQ_MEM_RECLAIM | WQ_FREEZABLE,
+									name);

Why add these two WQ_ flags? Also, whitespace is kinda funky.

include/linux/padata.h +0 -17

-/**
- * struct parallel_data - Internal control structure, covers everything
- * that depends on the cpumask in use.
- *
- * @ps: padata_shell object.
- * @reorder_list: percpu reorder lists
- * @squeue: percpu padata queues used for serialuzation.
- * @refcnt: Number of objects holding a reference on this parallel_data.
- * @seq_nr: Sequence number of the parallelized data object.
- * @processed: Number of already processed objects.
- * @cpu: Next CPU to be processed.
- * @cpumask: The cpumasks in use for parallel and serial workers.
- * @reorder_work: work struct for reordering.
- * @lock: Reorder lock.
- */
-struct parallel_data {
-	struct padata_shell		*ps;

Quoted 17 lines · Wang Jinchao

include/linux/padata.h +0 -17

-/**
- * struct parallel_data - Internal control structure, covers everything
- * that depends on the cpumask in use.
- *
- * @ps: padata_shell object.
- * @reorder_list: percpu reorder lists
- * @squeue: percpu padata queues used for serialuzation.
- * @refcnt: Number of objects holding a reference on this parallel_data.
- * @seq_nr: Sequence number of the parallelized data object.
- * @processed: Number of already processed objects.
- * @cpu: Next CPU to be processed.
- * @cpumask: The cpumasks in use for parallel and serial workers.
- * @reorder_work: work struct for reordering.
- * @lock: Reorder lock.
- */
-struct parallel_data {
-	struct padata_shell		*ps;

include/linux/padata.h line 107

Quoted 10 lines

include/linux/padata.h +0 -10

-	struct padata_list		__percpu *reorder_list;
-	struct padata_serial_queue	__percpu *squeue;
-	refcount_t			refcnt;
-	unsigned int			seq_nr;
-	unsigned int			processed;
-	int				cpu;
-	struct padata_cpumask		cpumask;
-	struct work_struct		reorder_work;
-	spinlock_t                      ____cacheline_aligned lock;
-};

reorder_list used to serialize one sequence of objects per padata_shell, but now serial_wq serializes all sequences of objects in one list of work_structs. That works in theory, since a total order can maintain each sequence's order, but it's possible (not sure yet, need to think more) that this could lead to deadlocks or other issues in odd cases such as the one that padata_shell was introduced for in bbefa1dd6a6d ("crypto: pcrypt - Avoid deadlock by using per-instance padata queues").

Quoted 29 lines · Daniel Jordan

Quoted 17 lines · Wang Jinchao

include/linux/padata.h +0 -17

-/**
- * struct parallel_data - Internal control structure, covers everything
- * that depends on the cpumask in use.
- *
- * @ps: padata_shell object.
- * @reorder_list: percpu reorder lists
- * @squeue: percpu padata queues used for serialuzation.
- * @refcnt: Number of objects holding a reference on this parallel_data.
- * @seq_nr: Sequence number of the parallelized data object.
- * @processed: Number of already processed objects.
- * @cpu: Next CPU to be processed.
- * @cpumask: The cpumasks in use for parallel and serial workers.
- * @reorder_work: work struct for reordering.
- * @lock: Reorder lock.
- */
-struct parallel_data {
-	struct padata_shell		*ps;

include/linux/padata.h line 107

Quoted 10 lines

include/linux/padata.h +0 -10

-	struct padata_list		__percpu *reorder_list;
-	struct padata_serial_queue	__percpu *squeue;
-	refcount_t			refcnt;
-	unsigned int			seq_nr;
-	unsigned int			processed;
-	int				cpu;
-	struct padata_cpumask		cpumask;
-	struct work_struct		reorder_work;
-	spinlock_t                      ____cacheline_aligned lock;
-};

Quoted 8 lines

reorder_list used to serialize one sequence of objects per padata_shell, but now serial_wq serializes all sequences of objects in one list of work_structs. That works in theory, since a total order can maintain each sequence's order, but it's possible (not sure yet, need to think more) that this could lead to deadlocks or other issues in odd cases such as the one that padata_shell was introduced for in bbefa1dd6a6d ("crypto: pcrypt - Avoid deadlock by using per-instance padata queues").

Yes, you are correct. This version is not only ordered at the padata_shell level but at the instance level, which indeed doesn't align with the design. Apart from potential deadlocks, it may also cause a padata_shell_B that should have completed earlier to be blocked by an unrelated padata_shell_B. I will address this issue in subsequent patches.