| Message ID | 20241112-refactor-blk-affinity-helpers-v3-1-573bfca0cbd8@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk: refactor queue affinity helpers | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On 11/12/24 14:26, Daniel Wagner wrote: > Introducing a callback in struct bus_type so that a subsystem > can hook up the getters directly. This approach avoids exposing > random getters in any subsystems APIs. > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Daniel Wagner <wagi@kernel.org> > --- > include/linux/device/bus.h | 3 +++ > 1 file changed, 3 insertions(+) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Han nes
On 12/11/2024 13:26, Daniel Wagner wrote: > Introducing a callback in struct bus_type so that a subsystem > can hook up the getters directly. This approach avoids exposing > random getters in any subsystems APIs. > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Daniel Wagner <wagi@kernel.org> > --- > include/linux/device/bus.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h > index cdc4757217f9bb4b36b5c3b8a48bab45737e44c5..b18658bce2c3819fc1cbeb38fb98391d56ec3317 100644 > --- a/include/linux/device/bus.h > +++ b/include/linux/device/bus.h > @@ -48,6 +48,7 @@ struct fwnode_handle; > * will never get called until they do. > * @remove: Called when a device removed from this bus. My impression is that this would be better suited to "struct device_driver", but I assume that there is a good reason to add to "struct bus_type".
On Wed, Nov 13, 2024 at 10:16:32AM +0000, John Garry wrote: > On 12/11/2024 13:26, Daniel Wagner wrote: > > Introducing a callback in struct bus_type so that a subsystem > > can hook up the getters directly. This approach avoids exposing > > random getters in any subsystems APIs. > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > Signed-off-by: Daniel Wagner <wagi@kernel.org> > > --- > > include/linux/device/bus.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h > > index cdc4757217f9bb4b36b5c3b8a48bab45737e44c5..b18658bce2c3819fc1cbeb38fb98391d56ec3317 100644 > > --- a/include/linux/device/bus.h > > +++ b/include/linux/device/bus.h > > @@ -48,6 +48,7 @@ struct fwnode_handle; > > * will never get called until they do. > > * @remove: Called when a device removed from this bus. > > My impression is that this would be better suited to "struct device_driver", > but I assume that there is a good reason to add to "struct bus_type". I think the main reason to put it here is that most of the drivers are happy with the getter on bus level and don't need special treatment. We don't have to touch all the drivers to hookup a common getter, nor do we have to install a default handler when the driver doesn't specify one. Having the callback in struct bus_driver avoids this. Though Christoph suggested it, so I can only guess. But you bring up a good point, if we had also an irq_get_affinity callback in struct device_driver it would be possible for the hisi_sas v2 driver to provide a getter and blk_mq_hctx_map_queues could do: for (queue = 0; queue < qmap->nr_queues; queue++) { if (dev->driver->irq_get_affinity) mask = dev->driver->irq_get_affinity; else if (dev->bus->irq_get_affinity) mask = dev->bus->irq_get_affinity(dev, queue + offset); if (!mask) goto fallback; for_each_cpu(cpu, mask) qmap->mq_map[cpu] = qmap->queue_offset + queue; } and with this in place the open coded version in hisi_sas v2 can also be replaced. If no one objects, I go ahead and add the callback to struct device_driver.
On 13/11/2024 12:36, Daniel Wagner wrote: >>> @@ -48,6 +48,7 @@ struct fwnode_handle; >>> * will never get called until they do. >>> * @remove: Called when a device removed from this bus. >> My impression is that this would be better suited to "struct device_driver", >> but I assume that there is a good reason to add to "struct bus_type". > I think the main reason to put it here is that most of the drivers are > happy with the getter on bus level and don't need special treatment. We > don't have to touch all the drivers to hookup a common getter, nor do we > have to install a default handler when the driver doesn't specify one. > Having the callback in struct bus_driver avoids this. Though Christoph > suggested it, so I can only guess. > > But you bring up a good point, if we had also an irq_get_affinity > callback in struct device_driver it would be possible for the > hisi_sas v2 driver to provide a getter and blk_mq_hctx_map_queues could > do: > > for (queue = 0; queue < qmap->nr_queues; queue++) { > if (dev->driver->irq_get_affinity) > mask = dev->driver->irq_get_affinity; > else if (dev->bus->irq_get_affinity) > mask = dev->bus->irq_get_affinity(dev, queue + offset); > if (!mask) > goto fallback; > > for_each_cpu(cpu, mask) > qmap->mq_map[cpu] = qmap->queue_offset + queue; > } > > and with this in place the open coded version in hisi_sas v2 can also be > replaced. Yeah, I think that it could be plugged in like: diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c index 342d75f12051..5172af77a3f0 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c @@ -3636,6 +3636,7 @@ static struct platform_driver hisi_sas_v2_driver = { .name = DRV_NAME, .of_match_table = sas_v2_of_match, .acpi_match_table = ACPI_PTR(sas_v2_acpi_match), + .irq_get_affinity_mask = hisi_sas_v2_get_affinity_mask, }, }; > If no one objects, I go ahead and add the callback to struct > device_driver. I'd wait for Christoph and Greg to both agree. I was just wondering why we use bus_type.
On Wed, Nov 13, 2024 at 01:44:02PM +0000, John Garry wrote: > On 13/11/2024 12:36, Daniel Wagner wrote: > > > > @@ -48,6 +48,7 @@ struct fwnode_handle; > > > > * will never get called until they do. > > > > * @remove: Called when a device removed from this bus. > > > My impression is that this would be better suited to "struct device_driver", > > > but I assume that there is a good reason to add to "struct bus_type". > > I think the main reason to put it here is that most of the drivers are > > happy with the getter on bus level and don't need special treatment. We > > don't have to touch all the drivers to hookup a common getter, nor do we > > have to install a default handler when the driver doesn't specify one. > > Having the callback in struct bus_driver avoids this. Though Christoph > > suggested it, so I can only guess. > > > > But you bring up a good point, if we had also an irq_get_affinity > > callback in struct device_driver it would be possible for the > > hisi_sas v2 driver to provide a getter and blk_mq_hctx_map_queues could > > do: > > > > for (queue = 0; queue < qmap->nr_queues; queue++) { > > if (dev->driver->irq_get_affinity) > > mask = dev->driver->irq_get_affinity; > > else if (dev->bus->irq_get_affinity) > > mask = dev->bus->irq_get_affinity(dev, queue + offset); > > if (!mask) > > goto fallback; > > > > for_each_cpu(cpu, mask) > > qmap->mq_map[cpu] = qmap->queue_offset + queue; > > } > > > > and with this in place the open coded version in hisi_sas v2 can also be > > replaced. > > Yeah, I think that it could be plugged in like: > > diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c > b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c > index 342d75f12051..5172af77a3f0 100644 > --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c > +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c > @@ -3636,6 +3636,7 @@ static struct platform_driver hisi_sas_v2_driver = { > .name = DRV_NAME, > .of_match_table = sas_v2_of_match, > .acpi_match_table = ACPI_PTR(sas_v2_acpi_match), > + .irq_get_affinity_mask = hisi_sas_v2_get_affinity_mask, > }, > }; > > > > If no one objects, I go ahead and add the callback to struct > > device_driver. > > I'd wait for Christoph and Greg to both agree. I was just wondering why we > use bus_type. bus types are good to set it at a bus level so you don't have to explicitly set it at each-and-every-driver. Depends on what you want this to be, if it is a "all drivers of this bus type will have the same callback" then put it on the bus. otherwise if you are going to mix/match on a same bus, then put it in the driver structure. hope this helps, greg k-h
On 13/11/2024 13:54, Greg Kroah-Hartman wrote: >> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c >> b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c >> index 342d75f12051..5172af77a3f0 100644 >> --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c >> +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c >> @@ -3636,6 +3636,7 @@ static struct platform_driver hisi_sas_v2_driver = { >> .name = DRV_NAME, >> .of_match_table = sas_v2_of_match, >> .acpi_match_table = ACPI_PTR(sas_v2_acpi_match), >> + .irq_get_affinity_mask = hisi_sas_v2_get_affinity_mask, >> }, >> }; >> >> >>> If no one objects, I go ahead and add the callback to struct >>> device_driver. >> I'd wait for Christoph and Greg to both agree. I was just wondering why we >> use bus_type. > bus types are good to set it at a bus level so you don't have to > explicitly set it at each-and-every-driver. Depends on what you want > this to be, if it is a "all drivers of this bus type will have the same > callback" then put it on the bus. otherwise if you are going to > mix/match on a same bus, then put it in the driver structure. Understood, I think all drivers on same bus will use the same callback. FWIW, most drivers of interest are pci drivers, so I thought that it would simply be a change like this (for those drivers if we use struct device_driver): @@ -1442,6 +1455,7 @@ int __pci_register_driver(struct pci_driver *drv, struct module *owner, drv->driver.mod_name = mod_name; drv->driver.groups = drv->groups; drv->driver.dev_groups = drv->dev_groups; + drv->driver.irq_get_affinity = pci_device_irq_get_affinity; Thanks, John
On Wed, Nov 13, 2024 at 02:12:30PM +0000, John Garry wrote: > On 13/11/2024 13:54, Greg Kroah-Hartman wrote: > > > diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c > > > b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c > > > index 342d75f12051..5172af77a3f0 100644 > > > --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c > > > +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c > > > @@ -3636,6 +3636,7 @@ static struct platform_driver hisi_sas_v2_driver = { > > > .name = DRV_NAME, > > > .of_match_table = sas_v2_of_match, > > > .acpi_match_table = ACPI_PTR(sas_v2_acpi_match), > > > + .irq_get_affinity_mask = hisi_sas_v2_get_affinity_mask, > > > }, > > > }; > > > > > > > > > > If no one objects, I go ahead and add the callback to struct > > > > device_driver. > > > I'd wait for Christoph and Greg to both agree. I was just wondering why we > > > use bus_type. > > bus types are good to set it at a bus level so you don't have to > > explicitly set it at each-and-every-driver. Depends on what you want > > this to be, if it is a "all drivers of this bus type will have the same > > callback" then put it on the bus. otherwise if you are going to > > mix/match on a same bus, then put it in the driver structure. > > Understood, I think all drivers on same bus will use the same callback. > > FWIW, most drivers of interest are pci drivers, so I thought that it would > simply be a change like this (for those drivers if we use struct > device_driver): > > @@ -1442,6 +1455,7 @@ int __pci_register_driver(struct pci_driver > *drv, struct module *owner, > drv->driver.mod_name = mod_name; > drv->driver.groups = drv->groups; > drv->driver.dev_groups = drv->dev_groups; > + drv->driver.irq_get_affinity = pci_device_irq_get_affinity; Yes, you can do that too. But now you have a pointer-per-driver instead of just one-per-bus. It's not a big deal, but again if this is always going to be the same for everything on a bus, make it a bus pointer please. thanks, greg k-h
On Wed, Nov 13, 2024 at 02:54:23PM +0100, Greg Kroah-Hartman wrote: > bus types are good to set it at a bus level so you don't have to > explicitly set it at each-and-every-driver. Depends on what you want > this to be, if it is a "all drivers of this bus type will have the same > callback" then put it on the bus. otherwise if you are going to > mix/match on a same bus, then put it in the driver structure. ... and that is exactly the case here. The driver itself has no business being involved.
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h index cdc4757217f9bb4b36b5c3b8a48bab45737e44c5..b18658bce2c3819fc1cbeb38fb98391d56ec3317 100644 --- a/include/linux/device/bus.h +++ b/include/linux/device/bus.h @@ -48,6 +48,7 @@ struct fwnode_handle; * will never get called until they do. * @remove: Called when a device removed from this bus. * @shutdown: Called at shut-down time to quiesce the device. + * @irq_get_affinity: Get IRQ affinity mask for the device on this bus. * * @online: Called to put the device back online (after offlining it). * @offline: Called to put the device offline for hot-removal. May fail. @@ -87,6 +88,8 @@ struct bus_type { void (*sync_state)(struct device *dev); void (*remove)(struct device *dev); void (*shutdown)(struct device *dev); + const struct cpumask *(*irq_get_affinity)(struct device *dev, + unsigned int irq_vec); int (*online)(struct device *dev); int (*offline)(struct device *dev);