On Wed, Oct 25, 2023 at 02:07:18PM -0400, Daniel Jordan wrote: > Hello, > > On Thu, Sep 28, 2023 at 04:53:38PM +0800, Wang Jinchao wrote: > > 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. > > > - 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/20190906014029.3345-1-daniel.m.jordan@xxxxxxxxxx/ > When I wrote the email on September 28th, I only used the "pcrypt_aead01" test case from LTP. Thank you for you and Steffen's responses. I now have more test cases, but I haven't tested them yet. > > - 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. You've convinced me, and I agree with your stance on keeping the cpumask. So, using WQ_UNBOUND is the right choice, and this will be reflected in my upcoming patches. > > 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. Workqueue provides the alloc_ordered_workqueue method, which may be more suitable for serial workers. > > > 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 > > Great speedup. A test that runs many requests for a long time in parallel is > also good to run, such as [0]. > I will conduct the test as previously mentioned. > > @@ -986,57 +281,27 @@ struct padata_instance *padata_alloc(const char *name) > ... > > + 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. You're right, I need to adjust this part of the code.
