[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ffe09847-b923-48ad-977d-28948cc2acd7@intel.com>
Date: Wed, 28 Aug 2024 15:02:36 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Manoj Vishwanathan <manojvishy@...gle.com>, Tony Nguyen
<anthony.l.nguyen@...el.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<google-lan-reviews@...glegroups.com>, Marco Leogrande <leogrande@...gle.com>
Subject: Re: [Intel-wired-lan] [[PATCH v2 iwl-next] v2 3/4] idpf: convert
workqueues to unbound
On 8/26/2024 11:10 AM, Manoj Vishwanathan wrote:
> From: Marco Leogrande <leogrande@...gle.com>
>
> When a workqueue is created with `WQ_UNBOUND`, its work items are
> served by special worker-pools, whose host workers are not bound to
> any specific CPU. In the default configuration (i.e. when
> `queue_delayed_work` and friends do not specify which CPU to run the
> work item on), `WQ_UNBOUND` allows the work item to be executed on any
> CPU in the same node of the CPU it was enqueued on. While this
> solution potentially sacrifices locality, it avoids contention with
> other processes that might dominate the CPU time of the processor the
> work item was scheduled on.
>
> This is not just a theoretical problem: in a praticular scenario
Nit: s/praticular/particular/
> misconfigured process was hogging most of the time from CPU0, leaving
> less than 0.5% of its CPU time to the kworker. The IDPF workqueues
> that were using the kworker on CPU0 suffered large completion delays
> as a result, causing performance degradation, timeouts and eventual
> system crash.
>
Curious how the delay could result in a full system crash. That seems
like some other concurrency issue. I guess something like a Tx timeout
could happen though.
> Tested:
>
> * I have also run a manual test to gauge the performance
> improvement. The test consists of an antagonist process
> (`./stress --cpu 2`) consuming as much of CPU 0 as possible. This
> process is run under `taskset 01` to bind it to CPU0, and its
> priority is changed with `chrt -pQ 9900 10000 ${pid}` and
> `renice -n -20 ${pid}` after start.
>
> Then, the IDPF driver is forced to prefer CPU0 by editing all calls
> to `queue_delayed_work`, `mod_delayed_work`, etc... to use CPU 0.
>
> Finally, `ktraces` for the workqueue events are collected.
>
> Without the current patch, the antagonist process can force
> arbitrary delays between `workqueue_queue_work` and
> `workqueue_execute_start`, that in my tests were as high as
> `30ms`. With the current patch applied, the workqueue can be
> migrated to another unloaded CPU in the same node, and, keeping
> everything else equal, the maximum delay I could see was `6us`.
>
Hmm. I don't have a direct issue with using WQ_UNBOUND, and I can't
think of any reason these work queue tasks *need* to be CPU bound.
I do feel like there may be other solutions to managing the tasks on the
system such that this isn't necessary.
However, if using WQ_UNBOUND solves these problems and is simpler in
that system administrators are less likely to screw things up, I think
its a net positive.
I do not know if there are any other side effects of WQ_UNBOUND, so take
this with a grain of salt:
Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> Fixes: 0fe45467a1041 (idpf: add create vport and netdev configuration)
> Signed-off-by: Marco Leogrande <leogrande@...gle.com>
> Signed-off-by: Manoj Vishwanathan <manojvishy@...gle.com>
> ---
> drivers/net/ethernet/intel/idpf/idpf_main.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
> index db476b3314c8..dfd56fc5ff65 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_main.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
> @@ -174,7 +174,8 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> pci_set_master(pdev);
> pci_set_drvdata(pdev, adapter);
>
> - adapter->init_wq = alloc_workqueue("%s-%s-init", 0, 0,
> + adapter->init_wq = alloc_workqueue("%s-%s-init",
> + WQ_UNBOUND | WQ_MEM_RECLAIM, 0,
> dev_driver_string(dev),
> dev_name(dev));
> if (!adapter->init_wq) {
> @@ -183,7 +184,8 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err_free;
> }
>
> - adapter->serv_wq = alloc_workqueue("%s-%s-service", 0, 0,
> + adapter->serv_wq = alloc_workqueue("%s-%s-service",
> + WQ_UNBOUND | WQ_MEM_RECLAIM, 0,
> dev_driver_string(dev),
> dev_name(dev));
> if (!adapter->serv_wq) {
> @@ -192,7 +194,8 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err_serv_wq_alloc;
> }
>
> - adapter->mbx_wq = alloc_workqueue("%s-%s-mbx", 0, 0,
> + adapter->mbx_wq = alloc_workqueue("%s-%s-mbx",
> + WQ_UNBOUND | WQ_MEM_RECLAIM, 0,
> dev_driver_string(dev),
> dev_name(dev));
> if (!adapter->mbx_wq) {
> @@ -201,7 +204,8 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err_mbx_wq_alloc;
> }
>
> - adapter->stats_wq = alloc_workqueue("%s-%s-stats", 0, 0,
> + adapter->stats_wq = alloc_workqueue("%s-%s-stats",
> + WQ_UNBOUND | WQ_MEM_RECLAIM, 0,
> dev_driver_string(dev),
> dev_name(dev));
> if (!adapter->stats_wq) {
> @@ -210,7 +214,8 @@ static int idpf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> goto err_stats_wq_alloc;
> }
>
> - adapter->vc_event_wq = alloc_workqueue("%s-%s-vc_event", 0, 0,
> + adapter->vc_event_wq = alloc_workqueue("%s-%s-vc_event",
> + WQ_UNBOUND | WQ_MEM_RECLAIM, 0,
> dev_driver_string(dev),
> dev_name(dev));
> if (!adapter->vc_event_wq) {
This seems like quite a lot of work queues for a driver :D
Powered by blists - more mailing lists