* [PATCH] genirq/affinity: Assign default affinity to pre/post vectors @ 2018-12-31 3:03 Huacai Chen 2019-01-15 19:10 ` Thomas Gleixner 0 siblings, 1 reply; 7+ messages in thread From: Huacai Chen @ 2018-12-31 3:03 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-kernel, Fuxin Zhang, Zhangjin Wu, Huacai Chen, stable, Christoph Hellwig, Michael Hernandez Generally, irq_create_affinity_masks() assign default affinity to pre/ post vectors correctly. However, it ignore the case that there are only pre/post vectors (when nvecs == affd->pre_vectors + affd->post_vectors) and return NULL. This case usually happens when nvecs = 1 (e.g. in nvme driver when MSI-X is unavailable and fallback to MSI) and will trigger the warning in pci_irq_get_affinity(). This patch fix the corner case. Fixes: 6f9a22bc5775d231ab8f ("PCI/MSI: Ignore affinity if pre/post vector count is more than min_vecs") Cc: stable@vger.kernel.org Cc: Christoph Hellwig <hch@lst.de> Cc: Michael Hernandez <michael.hernandez@cavium.com> Signed-off-by: Huacai Chen <chenhc@lemote.com> --- kernel/irq/affinity.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c index 45b68b4..9b766eb 100644 --- a/kernel/irq/affinity.c +++ b/kernel/irq/affinity.c @@ -240,13 +240,6 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd) struct irq_affinity_desc *masks = NULL; int i, nr_sets; - /* - * If there aren't any vectors left after applying the pre/post - * vectors don't bother with assigning affinity. - */ - if (nvecs == affd->pre_vectors + affd->post_vectors) - return NULL; - node_to_cpumask = alloc_node_to_cpumask(); if (!node_to_cpumask) return NULL; @@ -255,6 +248,17 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd) if (!masks) goto outnodemsk; + /* + * If there aren't any vectors left after applying the pre/post + * vectors then just assign the default affinity to all vectors. + */ + if (nvecs == affd->pre_vectors + affd->post_vectors) { + /* Fill all vectors that don't need affinity */ + for (curvec = 0; curvec < nvecs; curvec++) + cpumask_copy(&masks[curvec].mask, irq_default_affinity); + goto outnodemsk; + } + /* Fill out vectors at the beginning that don't need affinity */ for (curvec = 0; curvec < affd->pre_vectors; curvec++) cpumask_copy(&masks[curvec].mask, irq_default_affinity); -- 2.7.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors 2018-12-31 3:03 [PATCH] genirq/affinity: Assign default affinity to pre/post vectors Huacai Chen @ 2019-01-15 19:10 ` Thomas Gleixner 2019-01-16 3:00 ` 陈华才 0 siblings, 1 reply; 7+ messages in thread From: Thomas Gleixner @ 2019-01-15 19:10 UTC (permalink / raw) To: Huacai Chen Cc: linux-kernel, Fuxin Zhang, Zhangjin Wu, stable, Christoph Hellwig, Michael Hernandez On Mon, 31 Dec 2018, Huacai Chen wrote: > Generally, irq_create_affinity_masks() assign default affinity to pre/ > post vectors correctly. However, it ignore the case that there are only > pre/post vectors (when nvecs == affd->pre_vectors + affd->post_vectors) > and return NULL. This case usually happens when nvecs = 1 (e.g. in nvme > driver when MSI-X is unavailable and fallback to MSI) and will trigger > the warning in pci_irq_get_affinity(). This patch fix the corner case. Errm. This is just wrong. When this function returns NULL, then it has failed and the caller or any subsequent code is not supposed to use the result. The function can return NULL for other reasons, e.g. when the memory allocation failed. How are you going to duct tape that one? Thanks, tglx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors 2019-01-15 19:10 ` Thomas Gleixner @ 2019-01-16 3:00 ` 陈华才 2019-01-16 9:26 ` Thomas Gleixner 0 siblings, 1 reply; 7+ messages in thread From: 陈华才 @ 2019-01-16 3:00 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-kernel, Fuxin Zhang, wuzhangjin, stable, Christoph Hellwig, Michael Hernandez Hi, Thomas, I'm not removing all return NULL of irq_create_affinity_masks(), so the memory allocation failure still return NULL. I just handle the case that there are not enough irq vectors. E.g. in nvme driver, the caller may call irq_create_affinity_masks() with nvecs=1,pre_vectors=1,post_vectors=0. In this case, the only one vector's default affinity assigning is skipped. Huacai ------------------ Original ------------------ From: "Thomas Gleixner"<tglx@linutronix.de>; Date: Wed, Jan 16, 2019 03:10 AM To: "Huacai Chen"<chenhc@lemote.com>; Cc: "linux-kernel"<linux-kernel@vger.kernel.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "wuzhangjin"<wuzhangjin@gmail.com>; "stable"<stable@vger.kernel.org>; "Christoph Hellwig"<hch@lst.de>; "Michael Hernandez"<michael.hernandez@cavium.com>; Subject: Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors On Mon, 31 Dec 2018, Huacai Chen wrote: > Generally, irq_create_affinity_masks() assign default affinity to pre/ > post vectors correctly. However, it ignore the case that there are only > pre/post vectors (when nvecs == affd->pre_vectors + affd->post_vectors) > and return NULL. This case usually happens when nvecs = 1 (e.g. in nvme > driver when MSI-X is unavailable and fallback to MSI) and will trigger > the warning in pci_irq_get_affinity(). This patch fix the corner case. Errm. This is just wrong. When this function returns NULL, then it has failed and the caller or any subsequent code is not supposed to use the result. The function can return NULL for other reasons, e.g. when the memory allocation failed. How are you going to duct tape that one? Thanks, tglx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors 2019-01-16 3:00 ` 陈华才 @ 2019-01-16 9:26 ` Thomas Gleixner 2019-01-17 1:14 ` 陈华才 0 siblings, 1 reply; 7+ messages in thread From: Thomas Gleixner @ 2019-01-16 9:26 UTC (permalink / raw) To: 陈华才 Cc: linux-kernel, Fuxin Zhang, wuzhangjin, stable, Christoph Hellwig, Michael Hernandez [-- Attachment #1: Type: text/plain, Size: 2465 bytes --] Chen, On Wed, 16 Jan 2019, 陈华才 wrote: please do not top-post and use line breaks around 78 char. > I'm not removing all return NULL of irq_create_affinity_masks(), so the ...... Moved content to the place where it belongs so the context is preserved. > ------------------ Original ------------------ > From: "Thomas Gleixner"<tglx@linutronix.de>; > Date: Wed, Jan 16, 2019 03:10 AM > To: "Huacai Chen"<chenhc@lemote.com>; > Cc: "linux-kernel"<linux-kernel@vger.kernel.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "wuzhangjin"<wuzhangjin@gmail.com>; "stable"<stable@vger.kernel.org>; "Christoph Hellwig"<hch@lst.de>; "Michael Hernandez"<michael.hernandez@cavium.com>; > Subject: Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors And please configure your e-mail client to NOT copy the full headers into the reply. > > On Mon, 31 Dec 2018, Huacai Chen wrote: > > > > > Generally, irq_create_affinity_masks() assign default affinity to pre/ > > > post vectors correctly. However, it ignore the case that there are only > > > pre/post vectors (when nvecs == affd->pre_vectors + affd->post_vectors) > > > and return NULL. This case usually happens when nvecs = 1 (e.g. in nvme > > > driver when MSI-X is unavailable and fallback to MSI) and will trigger > > > the warning in pci_irq_get_affinity(). This patch fix the corner case. > > > > Errm. This is just wrong. When this function returns NULL, then it has > > failed and the caller or any subsequent code is not supposed to use the > > result. > > > > The function can return NULL for other reasons, e.g. when the memory > > allocation failed. How are you going to duct tape that one? > > I'm not removing all return NULL of irq_create_affinity_masks(), so the > memory allocation failure still return NULL. I just handle the case that > there are not enough irq vectors. E.g. in nvme driver, the caller may call > irq_create_affinity_masks() with nvecs=1,pre_vectors=1,post_vectors=0. In > this case, the only one vector's default affinity assigning is skipped. I did not say that you removed all NULL returns. I said that this function can return NULL for other reasons and then the same situation will happen. If the masks pointer returned is NULL then the calling code or any subsequent usage needs to handle it properly. Yes, I understand that this change makes the warning go away for that particular case, but that's not making it any more correct. Thanks, tglx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors 2019-01-16 9:26 ` Thomas Gleixner @ 2019-01-17 1:14 ` 陈华才 2019-01-18 1:52 ` Huacai Chen 0 siblings, 1 reply; 7+ messages in thread From: 陈华才 @ 2019-01-17 1:14 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-kernel, Fuxin Zhang, wuzhangjin, stable, Christoph Hellwig, Michael Hernandez, Huacai Chen 陈华才 江苏中科梦兰电子科技有限公司/自主安全事业部/软件部 江苏常熟虞山镇梦兰村 ------------------ Original ------------------ From: "Thomas Gleixner"<tglx@linutronix.de>; Date: Wed, Jan 16, 2019 05:26 PM To: "陈华才"<chenhc@lemote.com>; Cc: "linux-kernel"<linux-kernel@vger.kernel.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "wuzhangjin"<wuzhangjin@gmail.com>; "stable"<stable@vger.kernel.org>; "Christoph Hellwig"<hch@lst.de>; "Michael Hernandez"<michael.hernandez@cavium.com>; Subject: Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors Chen, On Wed, 16 Jan 2019, 陈华才 wrote: > please do not top-post and use line breaks around 78 char. > I'm not removing all return NULL of irq_create_affinity_masks(), so the ...... > > Moved content to the place where it belongs so the context is preserved. > ------------------ Original ------------------ > From: "Thomas Gleixner"<tglx@linutronix.de>; > Date: Wed, Jan 16, 2019 03:10 AM > To: "Huacai Chen"<chenhc@lemote.com>; > Cc: "linux-kernel"<linux-kernel@vger.kernel.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "wuzhangjin"<wuzhangjin@gmail.com>; "stable"<stable@vger.kernel.org>; "Christoph Hellwig"<hch@lst.de>; "Michael Hernandez"<michael.hernandez@cavium.com>; > Subject: Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors > And please configure your e-mail client to NOT copy the full headers into > the reply. > > > On Mon, 31 Dec 2018, Huacai Chen wrote: > > > > > > > Generally, irq_create_affinity_masks() assign default affinity to pre/ > > > > post vectors correctly. However, it ignore the case that there are only > > > > pre/post vectors (when nvecs == affd->pre_vectors + affd->post_vectors) > > > > and return NULL. This case usually happens when nvecs = 1 (e.g. in nvme > > > > driver when MSI-X is unavailable and fallback to MSI) and will trigger > > > > the warning in pci_irq_get_affinity(). This patch fix the corner case. > > > > > > Errm. This is just wrong. When this function returns NULL, then it has > > > failed and the caller or any subsequent code is not supposed to use the > > > result. > > > > > > The function can return NULL for other reasons, e.g. when the memory > > > allocation failed. How are you going to duct tape that one? > > > > I'm not removing all return NULL of irq_create_affinity_masks(), so the > > memory allocation failure still return NULL. I just handle the case that > > there are not enough irq vectors. E.g. in nvme driver, the caller may call > > irq_create_affinity_masks() with nvecs=1,pre_vectors=1,post_vectors=0. In > > this case, the only one vector's default affinity assigning is skipped. > > I did not say that you removed all NULL returns. I said that this function > can return NULL for other reasons and then the same situation will happen. > > If the masks pointer returned is NULL then the calling code or any > subsequent usage needs to handle it properly. Yes, I understand that this > change makes the warning go away for that particular case, but that's not > making it any more correct. Hi, Thomas, I don't think "nvecs == affd->pre_vectors + affd->post_vectors" is an ERROR, so it should be different with "return NULL for other reasons" to the caller. If the caller fallback from MSI-X to MSI, it is probably "nvecs=1,pre_vectors=1, post_vectors=0". The caller can work perfectly, if pre/post vectors are filled with the default affinity. > Thanks, > tglx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors 2019-01-17 1:14 ` 陈华才 @ 2019-01-18 1:52 ` Huacai Chen 2019-01-22 21:04 ` Thomas Gleixner 0 siblings, 1 reply; 7+ messages in thread From: Huacai Chen @ 2019-01-18 1:52 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-kernel, Fuxin Zhang, wuzhangjin, stable, Christoph Hellwig, Michael Hernandez On Thu, Jan 17, 2019 at 9:14 AM 陈华才 <chenhc@lemote.com> wrote: > > 陈华才 > 江苏中科梦兰电子科技有限公司/自主安全事业部/软件部 > 江苏常熟虞山镇梦兰村 > > > > ------------------ Original ------------------ > From: "Thomas Gleixner"<tglx@linutronix.de>; > Date: Wed, Jan 16, 2019 05:26 PM > To: "陈华才"<chenhc@lemote.com>; > Cc: "linux-kernel"<linux-kernel@vger.kernel.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "wuzhangjin"<wuzhangjin@gmail.com>; "stable"<stable@vger.kernel.org>; "Christoph Hellwig"<hch@lst.de>; "Michael Hernandez"<michael.hernandez@cavium.com>; > Subject: Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors > > Chen, > > On Wed, 16 Jan 2019, 陈华才 wrote: > > > please do not top-post and use line breaks around 78 char. > > > I'm not removing all return NULL of irq_create_affinity_masks(), so the ...... > > > > Moved content to the place where it belongs so the context is preserved. > > > ------------------ Original ------------------ > > From: "Thomas Gleixner"<tglx@linutronix.de>; > > Date: Wed, Jan 16, 2019 03:10 AM > > To: "Huacai Chen"<chenhc@lemote.com>; > > Cc: "linux-kernel"<linux-kernel@vger.kernel.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "wuzhangjin"<wuzhangjin@gmail.com>; "stable"<stable@vger.kernel.org>; "Christoph Hellwig"<hch@lst.de>; "Michael Hernandez"<michael.hernandez@cavium.com>; > > Subject: Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors > > > And please configure your e-mail client to NOT copy the full headers into > > the reply. > > > > > On Mon, 31 Dec 2018, Huacai Chen wrote: > > > > > > > > > Generally, irq_create_affinity_masks() assign default affinity to pre/ > > > > > post vectors correctly. However, it ignore the case that there are only > > > > > pre/post vectors (when nvecs == affd->pre_vectors + affd->post_vectors) > > > > > and return NULL. This case usually happens when nvecs = 1 (e.g. in nvme > > > > > driver when MSI-X is unavailable and fallback to MSI) and will trigger > > > > > the warning in pci_irq_get_affinity(). This patch fix the corner case. > > > > > > > > Errm. This is just wrong. When this function returns NULL, then it has > > > > failed and the caller or any subsequent code is not supposed to use the > > > > result. > > > > > > > > The function can return NULL for other reasons, e.g. when the memory > > > > allocation failed. How are you going to duct tape that one? > > > > > > I'm not removing all return NULL of irq_create_affinity_masks(), so the > > > memory allocation failure still return NULL. I just handle the case that > > > there are not enough irq vectors. E.g. in nvme driver, the caller may call > > > irq_create_affinity_masks() with nvecs=1,pre_vectors=1,post_vectors=0. In > > > this case, the only one vector's default affinity assigning is skipped. > > > > I did not say that you removed all NULL returns. I said that this function > > can return NULL for other reasons and then the same situation will happen. > > > > If the masks pointer returned is NULL then the calling code or any > > subsequent usage needs to handle it properly. Yes, I understand that this > > change makes the warning go away for that particular case, but that's not > > making it any more correct. Hi, Thomas, I don't think "nvecs == affd->pre_vectors + affd->post_vectors" is an ERROR, so it should be different with "return NULL for other reasons" to the caller. If the caller fallback from MSI-X to MSI, it is probably "nvecs=1,pre_vectors=1, post_vectors=0". The caller can work perfectly, if pre/post vectors are filled with the default affinity. Huacai > > Thanks, > > > tglx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] genirq/affinity: Assign default affinity to pre/post vectors 2019-01-18 1:52 ` Huacai Chen @ 2019-01-22 21:04 ` Thomas Gleixner 0 siblings, 0 replies; 7+ messages in thread From: Thomas Gleixner @ 2019-01-22 21:04 UTC (permalink / raw) To: Huacai Chen Cc: linux-kernel, Fuxin Zhang, wuzhangjin, Christoph Hellwig, Michael Hernandez, Jens Axboe, Bjorn Helgaas, Keith Busch, Ming Lei, linux-block, linux-nvme Chen, On Fri, 18 Jan 2019, Huacai Chen wrote: > > > I did not say that you removed all NULL returns. I said that this function > > > can return NULL for other reasons and then the same situation will happen. > > > > > > If the masks pointer returned is NULL then the calling code or any > > > subsequent usage needs to handle it properly. Yes, I understand that this > > > change makes the warning go away for that particular case, but that's not > > > making it any more correct. > > Hi, Thomas, > > I don't think "nvecs == affd->pre_vectors + affd->post_vectors" is an ERROR, > so it should be different with "return NULL for other reasons" to the caller. If > the caller fallback from MSI-X to MSI, it is probably "nvecs=1,pre_vectors=1, > post_vectors=0". The caller can work perfectly, if pre/post vectors are filled > with the default affinity. This is not about 'works'. This is about correctness. So again: The semantics of that function is, that it returns NULL on error. The reason for this NULL return is entirely irrelevant for the moment. If the calling code or any subsequent code proceeds as if nothing happened and later complains about it being NULL, then that logic at the calling or subsequent code is broken. And just making one particular error case not return NULL does not make it less broken because the function still can return NULL. So that proposed 'fix' is sunshine programming at best. Now for the change you are proposing. It's semantically wrong in the face of multiqueue devices. You are trying to make exactly one particular corner case "work" by some dubious definition of work: nvecs=1,pre_vectors=1,post_vectors=0 If pre + post != 1 then this still returns NULL and the same wreckage happens again. The point is that if there are not enough vectors to have at least one queue vector aside of pre and post then the whole queue management logic does not make any sense. This needs to be fixed elsewhere and not duct tape in the core logic with the argument 'works for me'. Thanks, tglx ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-22 21:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-31 3:03 [PATCH] genirq/affinity: Assign default affinity to pre/post vectors Huacai Chen 2019-01-15 19:10 ` Thomas Gleixner 2019-01-16 3:00 ` 陈华才 2019-01-16 9:26 ` Thomas Gleixner 2019-01-17 1:14 ` 陈华才 2019-01-18 1:52 ` Huacai Chen 2019-01-22 21:04 ` Thomas Gleixner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).