Re: [PATCH v10 2/5] s390/pci: Reuse FMB buffer across enable and disable cycles

[Gerd Bayer <gbayer@linux.ibm.com>]

On Tue, 2026-10-20 at 17:24 -0400, Omar Elghoul wrote:
> Allocate the FMB once upon first enable and keep it for the lifetime of
> the zdev instead of freeing it on every disable. Track the enablement
> state in the new statistics_on bool, since a non-null FMB no longer
> implies that measurement is enabled.

Hi Omar,

thanks for picking up the suggestion to split this into two patches.
However, if you swapped patch 2 and 3 you'd introduce the "bool
statistics_on" in a more intuitive way and then the reuse of FMB
buffers once they're allocated comes in more naturally? That way the
name of "statistics_on" has a better explanation and this patch becomes
a lot smaller, maybe.

> Defer freeing the buffer until device teardown in zpci_release_device().
> 
> Drop the alignment warning after allocating the FMB buffer due to it
> already being guaranteed by the kmem_cache.
> 
> Audit the only consumer of the FMB (pci_perf_show) and update its
> liveness check to be aware of this change.
> 
> Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
> ---
>  arch/s390/include/asm/pci.h |  1 +
>  arch/s390/pci/pci.c         | 41 ++++++++++++++++++++++---------------
>  arch/s390/pci/pci_debug.c   |  2 +-
>  3 files changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 88a125b92bdd..f4fea1c34746 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -208,6 +208,7 @@ struct zpci_dev {
>  	struct zpci_fmb *fmb;
>  	u16		fmb_update;	/* update interval */
>  	u16		fmb_length;
> +	bool		statistics_on;
>  
>  	u8		version;
>  	enum pci_bus_speed max_bus_speed;
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index c055a9ad0972..2a7e1ca0fa12 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -175,13 +175,17 @@ int zpci_fmb_enable_device(struct zpci_dev *zdev)
>  
>  	lockdep_assert_held(&zdev->fmb_lock);
>  
> -	if (zdev->fmb || sizeof(*zdev->fmb) < zdev->fmb_length)
> +	if (zdev->statistics_on || sizeof(*zdev->fmb) < zdev->fmb_length)
>  		return -EINVAL;
>  
> -	zdev->fmb = kmem_cache_zalloc(zdev_fmb_cache, GFP_KERNEL);
> -	if (!zdev->fmb)
> -		return -ENOMEM;
> -	WARN_ON((u64) zdev->fmb & 0xf);
> +	if (!zdev->fmb) {
> +		zdev->fmb = kmem_cache_zalloc(zdev_fmb_cache, GFP_KERNEL);
> +		if (!zdev->fmb)
> +			return -ENOMEM;
> +	} else {
> +		/* reuse the same FMB buffer for as long as the zdev lives */
> +		memset(zdev->fmb, 0, sizeof(*zdev->fmb));
> +	}
>  
>  	/* reset software counters */
>  	spin_lock_irqsave(&zdev->dom_lock, flags);
> @@ -199,11 +203,11 @@ int zpci_fmb_enable_device(struct zpci_dev *zdev)
>  	fib.fmb_addr = virt_to_phys(zdev->fmb);
>  	fib.gd = zdev->gisa;
>  	cc = zpci_mod_fc(req, &fib, &status);
> -	if (cc) {
> -		kmem_cache_free(zdev_fmb_cache, zdev->fmb);
> -		zdev->fmb = NULL;
> -	}
> -	return cc ? -EIO : 0;
> +	if (cc)
> +		return -EIO;
> +
> +	zdev->statistics_on = true;
> +	return 0;
>  }
>  
>  /* Modify PCI: Disable PCI function measurement */
> @@ -215,7 +219,7 @@ int zpci_fmb_disable_device(struct zpci_dev *zdev)
>  
>  	lockdep_assert_held(&zdev->fmb_lock);
>  
> -	if (!zdev->fmb)
> +	if (!zdev->statistics_on)
>  		return -EINVAL;
>  
>  	fib.gd = zdev->gisa;
> @@ -224,12 +228,11 @@ int zpci_fmb_disable_device(struct zpci_dev *zdev)
>  	cc = zpci_mod_fc(req, &fib, &status);
>  	if (cc == 3) /* Function already gone. */
>  		cc = 0;
> +	if (cc)
> +		return -EIO;
>  
> -	if (!cc) {
> -		kmem_cache_free(zdev_fmb_cache, zdev->fmb);
> -		zdev->fmb = NULL;
> -	}
> -	return cc ? -EIO : 0;
> +	zdev->statistics_on = false;
> +	return 0;
>  }
>  
>  static int zpci_cfg_load(struct zpci_dev *zdev, int offset, u32 *val, u8 len)
> @@ -1003,6 +1006,12 @@ void zpci_release_device(struct kref *kref)
>  	if (zdev->has_resources)
>  		zpci_cleanup_bus_resources(zdev);
>  
> +	if (zdev->fmb) {
> +		zdev->statistics_on = false;
> +		kmem_cache_free(zdev_fmb_cache, zdev->fmb);
> +		zdev->fmb = NULL;
> +	}
> +
>  	zpci_bus_device_unregister(zdev);
>  	zpci_destroy_iommu(zdev);
>  	zpci_dbg(3, "rem fid:%x\n", zdev->fid);
> diff --git a/arch/s390/pci/pci_debug.c b/arch/s390/pci/pci_debug.c
> index c7ed7bf254b5..419c6a9200a2 100644
> --- a/arch/s390/pci/pci_debug.c
> +++ b/arch/s390/pci/pci_debug.c
> @@ -97,7 +97,7 @@ static int pci_perf_show(struct seq_file *m, void *v)
>  		return 0;
>  
>  	mutex_lock(&zdev->fmb_lock);
> -	if (!zdev->fmb) {
> +	if (!zdev->statistics_on || !zdev->fmb) {
>  		mutex_unlock(&zdev->fmb_lock);
>  		seq_puts(m, "FMB statistics disabled\n");
>  		return 0;

Thank you,
Gerd