Re: [PATCH v9 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement

[Gerd Bayer]

On Mon, 2026-10-12 at 13:41 -0400, Omar Elghoul wrote:
> On 10/12/26 5:22 AM, Gerd Bayer wrote:
> > On Wed, 2026-10-07 at 09:59 -0400, Omar Elghoul wrote:
> > > Don't free the FMB buffer when disabling measurement in
> > > zpci_fmb_disable_device(). Instead, make the buffer persistent for the
> > > lifetime of the device and reuse it across enable/disable cycles. Defer
> > > freeing the buffer until teardown in zpci_release_device().
> > 
> > I lost track if this was discussed before: Would it make sense to break
> > this out into a separate patch, so that memory allocation/life-time
> > questions are decoupled from function-measurement preservation?
> 
> I think we did discuss that before and I expressed a preference for
> leaving them in together, but since this patch seems to be growing I
> guess splitting it would make review easier.

Hi Omar,

reviewing is one argument, the others are: Debugging, bi-section,
fixing, back-porting, ...

   [ snip ]

> > 
> 
> > > +/* Re-enable PCI function measurement: clear the software IOMMU counters
> > > + * and (re-)enable the FMB. This function is used to restore the desired
> > > + * FMB enablement state when measurement may be implicitly disabled by
> > > + * firmware (i.e., via CLP in zpci_disable_device())
> > > + */
> > > +int zpci_fmb_reenable_device(struct zpci_dev *zdev)
> > > +{
> > > +	u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_SET_MEASURE);
> > > +	struct zpci_iommu_ctrs *ctrs;
> > > +	struct zpci_fib fib = {0};
> > > +	unsigned long flags;
> > > +	u8 cc, status;
> > > +
> > > +	lockdep_assert_held(&zdev->fmb_lock);
> > > +
> > > +	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 the zdev lives */
> > > +		memset(zdev->fmb, 0, sizeof(*zdev->fmb));
> > >   	}
> > > -	return cc ? -EIO : 0;
> > > +
> > > +	/* reset software counters */
> > > +	spin_lock_irqsave(&zdev->dom_lock, flags);
> > > +	ctrs = zpci_get_iommu_ctrs(zdev);
> > > +	if (ctrs) {
> > > +		atomic64_set(&ctrs->mapped_pages, 0);
> > > +		atomic64_set(&ctrs->unmapped_pages, 0);
> > > +		atomic64_set(&ctrs->global_rpcits, 0);
> > > +		atomic64_set(&ctrs->sync_map_rpcits, 0);
> > > +		atomic64_set(&ctrs->sync_rpcits, 0);
> > > +	}
> > > +	spin_unlock_irqrestore(&zdev->dom_lock, flags);
> > 
> > I know this is pre-existing, but how about breaking out the buffer and
> > counter preparation/clearing into a helper function?
> 
> I believe it has been done in a previous version of this series at which
> point we had two callers for it. Since we don't have another caller now
> I didn't think of splitting it up.

To me, the "detour" into clearing the zpci_iommu_ctrs together with its
additional locking requirements was the point where I felt: This
function is doing too many things :)

   [ snip ]
> > 

> > Oh, and while playing around with this patch (with and without my
> > additional modifications), I realized that statistics/function
> > measurement gets re-enabled when I run `zpcictl --reset-fw <bdf>` to a
> > function - despite it being switched off before the reset.
> 
> Great catch -- the FMB re-enablement in zpci_reenable_device() was just
> one of the two places that needed it, I missed to add it in
> pcibios_enable_device() instead of the existing unconditional enable. I
> will fix this in the next version.

It just occured to me, that if you plan to use the same conditional
enable based on fmb_requested/statistics_on in pcibios_enable_device()
you may want to change the initialization of that bool to "true" in
zpci_create_device() to keep the default behavior of the statistics
being on.

That said, I always felt a little uncomfortable with this default
setting, since it creates load on the overall system, even if nobody
cares about the data. Apparently, that default-enable was chosen at the
initial introduction of statistics with commit d0b0885316ab ("s390/pci:
performance statistics and debug infrastructure"). But that's a
separate discussion I need to have with Niklas and Halil

> re: splitting up the functions into a pair of request functions and a
> pair that actually do the MPCIFC: it will make resolving this issue more
> straightforward, because preserving the state in pcibios_enable_device()
> will require us to disable FMB without clearing its flag in
> pcibios_disable_device() as well, which is cleanly possible with the
> changes you suggested.
> 
> Thanks!
> 
> 
[ snip ]

Thank you!
Gerd