From: Thomas Gleixner <tglx@linutronix.de> To: Bjorn Helgaas <helgaas@kernel.org> Cc: Ming Lei <ming.lei@redhat.com>, Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org, Sagi Grimberg <sagi@grimberg.me>, linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org Subject: Re: [PATCH 2/5] genirq/affinity: allow driver to setup managed IRQ's affinity Date: Mon, 11 Feb 2019 23:38:07 +0100 (CET) [thread overview] Message-ID: <alpine.DEB.2.21.1902112322470.1654@nanos.tec.linutronix.de> (raw) In-Reply-To: <20190211143950.GA151039@google.com> Ming, On Mon, 11 Feb 2019, Bjorn Helgaas wrote: > On Mon, Feb 11, 2019 at 11:54:00AM +0800, Ming Lei wrote: > > On Sun, Feb 10, 2019 at 05:30:41PM +0100, Thomas Gleixner wrote: > > > On Fri, 25 Jan 2019, Ming Lei wrote: > > > > > > > This patch introduces callback of .setup_affinity into 'struct > > > > irq_affinity', so that: > > > > > > Please see Documentation/process/submitting-patches.rst. Search for 'This > > > patch' .... > > > > Sorry for that, because I am not a native English speaker and it looks a bit > > difficult for me to understand the subtle difference. Sorry I was a bit terse. > I think Thomas is saying that instead of "This patch introduces > callback ...", you could say "Introduce callback of ...". > > The changelog is *part* of the patch, so the context is obvious and > there's no need to include the words "This patch". > > I make the same changes to patches I receive. In fact, I would go > even further and say "Add callback .setup_affinity() ..." because "add" > means the same as "introduce" but is shorter and simpler. Thanks for the explanation, Bjorn! There is another point here. It's not only the 'This patch introduces ...' part. It's also good practice to structure the changelog so it provides context and reasoning first and then tells what the change is, e.g.: The current handling of multiple interrupt sets in the core interrupt affinity logic, requires the driver to do ....... This is necessary because .... This handling should be in the core code, but the core implementation has no way to recompute the interrupt sets for a given number of vectors. Add an optional callback to struct affd, which lets the driver recompute the interrupt set before the interrupt affinity spreading takes place. The first paragraph provides context, i.e. the status quo, The second paragraph provides reasoning what is missing and the last one tells what's done to solve it. That's pretty much the same for all changelogs, even if you fix a bug. Just in the bug case, the second paragraph describes the details of the bug and the possible consequences. You really need to look at the changelog as a stand alone information source. That's important when you look at a commit as an outsider or even if you look at your own patch 6 month down the road when you already paged out all the details. Hope that clarifies it. Thanks, tglx
WARNING: multiple messages have this Message-ID (diff)
From: tglx@linutronix.de (Thomas Gleixner) Subject: [PATCH 2/5] genirq/affinity: allow driver to setup managed IRQ's affinity Date: Mon, 11 Feb 2019 23:38:07 +0100 (CET) [thread overview] Message-ID: <alpine.DEB.2.21.1902112322470.1654@nanos.tec.linutronix.de> (raw) In-Reply-To: <20190211143950.GA151039@google.com> Ming, On Mon, 11 Feb 2019, Bjorn Helgaas wrote: > On Mon, Feb 11, 2019@11:54:00AM +0800, Ming Lei wrote: > > On Sun, Feb 10, 2019@05:30:41PM +0100, Thomas Gleixner wrote: > > > On Fri, 25 Jan 2019, Ming Lei wrote: > > > > > > > This patch introduces callback of .setup_affinity into 'struct > > > > irq_affinity', so that: > > > > > > Please see Documentation/process/submitting-patches.rst. Search for 'This > > > patch' .... > > > > Sorry for that, because I am not a native English speaker and it looks a bit > > difficult for me to understand the subtle difference. Sorry I was a bit terse. > I think Thomas is saying that instead of "This patch introduces > callback ...", you could say "Introduce callback of ...". > > The changelog is *part* of the patch, so the context is obvious and > there's no need to include the words "This patch". > > I make the same changes to patches I receive. In fact, I would go > even further and say "Add callback .setup_affinity() ..." because "add" > means the same as "introduce" but is shorter and simpler. Thanks for the explanation, Bjorn! There is another point here. It's not only the 'This patch introduces ...' part. It's also good practice to structure the changelog so it provides context and reasoning first and then tells what the change is, e.g.: The current handling of multiple interrupt sets in the core interrupt affinity logic, requires the driver to do ....... This is necessary because .... This handling should be in the core code, but the core implementation has no way to recompute the interrupt sets for a given number of vectors. Add an optional callback to struct affd, which lets the driver recompute the interrupt set before the interrupt affinity spreading takes place. The first paragraph provides context, i.e. the status quo, The second paragraph provides reasoning what is missing and the last one tells what's done to solve it. That's pretty much the same for all changelogs, even if you fix a bug. Just in the bug case, the second paragraph describes the details of the bug and the possible consequences. You really need to look at the changelog as a stand alone information source. That's important when you look at a commit as an outsider or even if you look at your own patch 6 month down the road when you already paged out all the details. Hope that clarifies it. Thanks, tglx
next prev parent reply other threads:[~2019-02-11 22:38 UTC|newest] Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-01-25 9:53 [PATCH 0/5] genirq/affinity: introduce .setup_affinity to support allocating interrupt sets Ming Lei 2019-01-25 9:53 ` Ming Lei 2019-01-25 9:53 ` [PATCH 1/5] genirq/affinity: move allocation of 'node_to_cpumask' to irq_build_affinity_masks Ming Lei 2019-01-25 9:53 ` Ming Lei 2019-02-07 22:02 ` Bjorn Helgaas 2019-02-07 22:02 ` Bjorn Helgaas 2019-02-10 19:01 ` [tip:irq/core] genirq/affinity: Move allocation of 'node_to_cpumask' to irq_build_affinity_masks() tip-bot for Ming Lei 2019-01-25 9:53 ` [PATCH 2/5] genirq/affinity: allow driver to setup managed IRQ's affinity Ming Lei 2019-01-25 9:53 ` Ming Lei 2019-02-07 22:21 ` Bjorn Helgaas 2019-02-07 22:21 ` Bjorn Helgaas 2019-02-10 9:22 ` Ming Lei 2019-02-10 9:22 ` Ming Lei 2019-02-10 16:30 ` Thomas Gleixner 2019-02-10 16:30 ` Thomas Gleixner 2019-02-11 3:54 ` Ming Lei 2019-02-11 3:54 ` Ming Lei 2019-02-11 14:39 ` Bjorn Helgaas 2019-02-11 14:39 ` Bjorn Helgaas 2019-02-11 22:38 ` Thomas Gleixner [this message] 2019-02-11 22:38 ` Thomas Gleixner 2019-02-12 11:17 ` Ming Lei 2019-02-12 11:17 ` Ming Lei 2019-01-25 9:53 ` [PATCH 3/5] genirq/affinity: introduce irq_build_affinity() Ming Lei 2019-01-25 9:53 ` Ming Lei 2019-01-25 9:53 ` [PATCH 4/5] nvme-pci: simplify nvme_setup_irqs() via .setup_affinity callback Ming Lei 2019-01-25 9:53 ` Ming Lei 2019-02-10 16:39 ` Thomas Gleixner 2019-02-10 16:39 ` Thomas Gleixner 2019-02-11 3:58 ` Ming Lei 2019-02-11 3:58 ` Ming Lei 2019-02-10 18:49 ` Thomas Gleixner 2019-02-10 18:49 ` Thomas Gleixner 2019-02-11 4:09 ` Ming Lei 2019-02-11 4:09 ` Ming Lei 2019-01-25 9:53 ` [PATCH 5/5] genirq/affinity: remove support for allocating interrupt sets Ming Lei 2019-01-25 9:53 ` Ming Lei 2019-02-07 22:22 ` Bjorn Helgaas 2019-02-07 22:22 ` Bjorn Helgaas 2019-01-25 9:56 ` [PATCH 0/5] genirq/affinity: introduce .setup_affinity to support " Ming Lei 2019-01-25 9:56 ` Ming Lei
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=alpine.DEB.2.21.1902112322470.1654@nanos.tec.linutronix.de \ --to=tglx@linutronix.de \ --cc=axboe@kernel.dk \ --cc=hch@lst.de \ --cc=helgaas@kernel.org \ --cc=linux-block@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=ming.lei@redhat.com \ --cc=sagi@grimberg.me \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.