| Message ID | 20240826073106.56918-7-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Code cleanup and CDL memory usage reduction | expand |
On Mon, Aug 26, 2024 at 04:31:05PM +0900, Damien Le Moal wrote: > The ncq_sense_buf buffer field of struct ata_port is allocated and used > only for devices that support command duration limits. So move this > field out of struct ata_port and into struct ata_device together with > the CDL log buffer. The ncq_sense_buf buf is currently only allocated if the device supports CDL, so the currently extra space that we take up in struct ata_port, for non-CDL devices is just an empty pointer. Additionally, sector_buf, which we use for reading all the log pages belongs to struct ata_port, not struct ata_device. If you also still keep sector_buf in struct ata_port, then I think that this change makes the code more inconsistent. I would suggest to either move both or move none. But even then I don't really see the value of moving this from struct ata_port to ata_device. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/ata/libata-core.c | 11 +++++------ > drivers/ata/libata-sata.c | 2 +- > drivers/ata/libata-transport.c | 3 +++ > include/linux/libata.h | 4 ++-- > 4 files changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index b5a051bbb01f..6a1d300dd1f5 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -2581,9 +2581,9 @@ static void ata_dev_config_cdl(struct ata_device *dev) > * policy set to 0xD (successful completion with sense data available > * bit set). > */ > - if (!ap->ncq_sense_buf) { > - ap->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL); > - if (!ap->ncq_sense_buf) > + if (!dev->ncq_sense_buf) { > + dev->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL); > + if (!dev->ncq_sense_buf) > goto not_supported; > } > > @@ -2604,8 +2604,8 @@ static void ata_dev_config_cdl(struct ata_device *dev) > > not_supported: > dev->flags &= ~(ATA_DFLAG_CDL | ATA_DFLAG_CDL_ENABLED); > - kfree(ap->ncq_sense_buf); > - ap->ncq_sense_buf = NULL; > + kfree(dev->ncq_sense_buf); > + dev->ncq_sense_buf = NULL; > } > > static int ata_dev_config_lba(struct ata_device *dev) > @@ -5462,7 +5462,6 @@ void ata_port_free(struct ata_port *ap) > > kfree(ap->pmp_link); > kfree(ap->slave_link); > - kfree(ap->ncq_sense_buf); > ida_free(&ata_ida, ap->print_id); > kfree(ap); > } > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index 020893da900d..50ea254a213d 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -1505,7 +1505,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link) > { > struct ata_device *dev = link->device; > struct ata_port *ap = dev->link->ap; > - u8 *buf = ap->ncq_sense_buf; > + u8 *buf = dev->ncq_sense_buf; > struct ata_queued_cmd *qc; > unsigned int err_mask, tag; > u8 *sense, sk = 0, asc = 0, ascq = 0; > diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c > index 474816a9efa1..14f50c91ceb9 100644 > --- a/drivers/ata/libata-transport.c > +++ b/drivers/ata/libata-transport.c > @@ -671,6 +671,9 @@ static int ata_tdev_match(struct attribute_container *cont, > */ > static void ata_tdev_free(struct ata_device *dev) > { > + kfree(dev->ncq_sense_buf); > + dev->ncq_sense_buf = NULL; > + I don't like that you are freeing ncq_sense_buf in ata_tdev_free(), a function that is use to free the sysfs entry for a struct ata_device. ata_tdev_add() and ata_tdev_delete() is about adding and removing the sysfs entry for a certain struct ata_device. It is not where the ata_device is allocated and freed, so it seems out of place to do free struct ata_device struct members here. ata_device is allocated when calling ata_port_alloc(). (struct ata_port has a "struct ata_link link;", and struct ata_link has a "struct ata_device device[ATA_MAX_DEVICES]", so struct ata_device is allocated by ata_port_alloc(), then initialized by ata_link_init(), which calls ata_dev_init().) If you want to free the ncq_sense_buf, then I think you should do so where we free the ata_device, which is currently when we free the struct ata_port. So I still think the best place to currently free this is in ata_port_free(). Considering both review comments, doesn't it make more sense to just keep ncq_sense_buf in struct ata_port? > transport_destroy_device(&dev->tdev); > put_device(&dev->tdev); > } > diff --git a/include/linux/libata.h b/include/linux/libata.h > index e07a9b5d45df..3fb6980c8aa1 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -762,7 +762,8 @@ struct ata_device { > /* Concurrent positioning ranges */ > struct ata_cpr_log *cpr_log; > > - /* Command Duration Limits log support */ > + /* Command Duration Limits support */ > + u8 *ncq_sense_buf; > u8 cdl[ATA_LOG_CDL_SIZE]; > > /* error history */ > @@ -915,7 +916,6 @@ struct ata_port { > struct ata_acpi_gtm __acpi_init_gtm; /* use ata_acpi_init_gtm() */ > #endif > /* owned by EH */ > - u8 *ncq_sense_buf; > u8 sector_buf[ATA_SECT_SIZE] ____cacheline_aligned; > }; > > -- > 2.46.0 >
On 8/26/24 23:48, Niklas Cassel wrote: > On Mon, Aug 26, 2024 at 04:31:05PM +0900, Damien Le Moal wrote: >> The ncq_sense_buf buffer field of struct ata_port is allocated and used >> only for devices that support command duration limits. So move this >> field out of struct ata_port and into struct ata_device together with >> the CDL log buffer. > > The ncq_sense_buf buf is currently only allocated if the device supports CDL, > so the currently extra space that we take up in struct ata_port, for non-CDL > devices is just an empty pointer. No, we have cdl descriptor log page buffer (ap->cdl) which takes ATA_LOG_CDL_SIZE (512 B). Furthermore, thinking of this some more, having that buffer attached to the port is totally wrong if we are dealing with a pmp attached device: we can have multiple devices supporting CDL that will all end up overwriting that buffer. So this is actually a bug: either we move the cdl log buffer to ata_device, or we must not probe for CDL in the case of a PMP attached device. The latter is fine I think as CDL is really an enterprise feature that is very unlikely to be used with consumer PMP. But the former is more logical. > Additionally, sector_buf, which we use for reading all the log pages belongs > to struct ata_port, not struct ata_device. Yes, but that buffer is only used in EH context when all devices attached to a port (1 for most cases but more than 1 for PMP) are idle. So this is fine. This buffer is not used at run-time. > If you also still keep sector_buf in struct ata_port, then I think that this > change makes the code more inconsistent. I would suggest to either move both > or move none. But even then I don't really see the value of moving this from > struct ata_port to ata_device. The justification of the move is above. My commit message did not reflect this though, so I will improve that. Also, it may make sense to squash this path with patch 7... Will see how that looks.
Hello Damien, On Tue, Aug 27, 2024 at 04:50:14PM +0900, Damien Le Moal wrote: > On 8/26/24 23:48, Niklas Cassel wrote: > > On Mon, Aug 26, 2024 at 04:31:05PM +0900, Damien Le Moal wrote: > >> The ncq_sense_buf buffer field of struct ata_port is allocated and used > >> only for devices that support command duration limits. So move this > >> field out of struct ata_port and into struct ata_device together with > >> the CDL log buffer. > > > > The ncq_sense_buf buf is currently only allocated if the device supports CDL, > > so the currently extra space that we take up in struct ata_port, for non-CDL > > devices is just an empty pointer. > > No, we have cdl descriptor log page buffer (ap->cdl) which takes > ATA_LOG_CDL_SIZE (512 B). Furthermore, thinking of this some more, having that > buffer attached to the port is totally wrong if we are dealing with a pmp > attached device: we can have multiple devices supporting CDL that will all end > up overwriting that buffer. So this is actually a bug: either we move the cdl > log buffer to ata_device, or we must not probe for CDL in the case of a PMP > attached device. The latter is fine I think as CDL is really an enterprise > feature that is very unlikely to be used with consumer PMP. But the former is > more logical. I think you are mixing things up, we don't have any ap->cdl. We have: 1) ap->ncq_sense_buf (u8 *ncq_sense_buf) 2) dev->cdl (u8 cdl[ATA_LOG_CDL_SIZE]) 3) ap->sector_buf (u8 sector_buf[ATA_SECT_SIZE]) I do not think there is any bug in having ap->ncq_sense_buf in struct ata_port, like the code currently does, because like you say, we currently only fetch the Successful NCQ Commands log from EH context, so this should be safe even when using PMPs. For dev->cdl, it is currently in struct ata_device, which is correct, because as you say, caching the CDL descriptor log page in the ata_port would not be correct, since it could then get overwritten by another device if using a PMP. (Changing this to be dynamically allocated would be a nice optimization.) For ap->sector_buf, I suggested to perhaps move this to ata_device, such that the ata_read_log() functions (which already take a ata_device as first argument) do no longer need to take a "buffer" argument, the buffer could be derived from the ata_device if we move the buffer there. If we move ap->sector_buf to struct ata_device, then I think that it makes sense to also move ap->ncq_sense_buf to struct ata_device. Kind regards, Niklas
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index b5a051bbb01f..6a1d300dd1f5 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -2581,9 +2581,9 @@ static void ata_dev_config_cdl(struct ata_device *dev) * policy set to 0xD (successful completion with sense data available * bit set). */ - if (!ap->ncq_sense_buf) { - ap->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL); - if (!ap->ncq_sense_buf) + if (!dev->ncq_sense_buf) { + dev->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL); + if (!dev->ncq_sense_buf) goto not_supported; } @@ -2604,8 +2604,8 @@ static void ata_dev_config_cdl(struct ata_device *dev) not_supported: dev->flags &= ~(ATA_DFLAG_CDL | ATA_DFLAG_CDL_ENABLED); - kfree(ap->ncq_sense_buf); - ap->ncq_sense_buf = NULL; + kfree(dev->ncq_sense_buf); + dev->ncq_sense_buf = NULL; } static int ata_dev_config_lba(struct ata_device *dev) @@ -5462,7 +5462,6 @@ void ata_port_free(struct ata_port *ap) kfree(ap->pmp_link); kfree(ap->slave_link); - kfree(ap->ncq_sense_buf); ida_free(&ata_ida, ap->print_id); kfree(ap); } diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 020893da900d..50ea254a213d 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1505,7 +1505,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link) { struct ata_device *dev = link->device; struct ata_port *ap = dev->link->ap; - u8 *buf = ap->ncq_sense_buf; + u8 *buf = dev->ncq_sense_buf; struct ata_queued_cmd *qc; unsigned int err_mask, tag; u8 *sense, sk = 0, asc = 0, ascq = 0; diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c index 474816a9efa1..14f50c91ceb9 100644 --- a/drivers/ata/libata-transport.c +++ b/drivers/ata/libata-transport.c @@ -671,6 +671,9 @@ static int ata_tdev_match(struct attribute_container *cont, */ static void ata_tdev_free(struct ata_device *dev) { + kfree(dev->ncq_sense_buf); + dev->ncq_sense_buf = NULL; + transport_destroy_device(&dev->tdev); put_device(&dev->tdev); } diff --git a/include/linux/libata.h b/include/linux/libata.h index e07a9b5d45df..3fb6980c8aa1 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -762,7 +762,8 @@ struct ata_device { /* Concurrent positioning ranges */ struct ata_cpr_log *cpr_log; - /* Command Duration Limits log support */ + /* Command Duration Limits support */ + u8 *ncq_sense_buf; u8 cdl[ATA_LOG_CDL_SIZE]; /* error history */ @@ -915,7 +916,6 @@ struct ata_port { struct ata_acpi_gtm __acpi_init_gtm; /* use ata_acpi_init_gtm() */ #endif /* owned by EH */ - u8 *ncq_sense_buf; u8 sector_buf[ATA_SECT_SIZE] ____cacheline_aligned; };
The ncq_sense_buf buffer field of struct ata_port is allocated and used only for devices that support command duration limits. So move this field out of struct ata_port and into struct ata_device together with the CDL log buffer. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-core.c | 11 +++++------ drivers/ata/libata-sata.c | 2 +- drivers/ata/libata-transport.c | 3 +++ include/linux/libata.h | 4 ++-- 4 files changed, 11 insertions(+), 9 deletions(-)