[RFC v2 0/1]s390/cio: remove uevent suppress from cio driver

[RFC v2 0/1]s390/cio: remove uevent suppress from cio driver

[Vineeth Vijayan]

This is the follow-up for the old RFC which i have posted here couple of
months back. During the previous discussions on that RFC we concluded
that removing the uevent-suppress from the CIO layer is the cleaner way
of doing it and we should proceed. Reason for this RFC is, i want to
convince myself that, i am not doing something wrong. I would like to
bring up some of the tests i have done and the conclusion from those
tests.

The reason for introducing the delay in uevent generation was to avoid a
uevent storm from those subchannels which does not have a valid device
connected on this. I think for the new generation Z machines, this is
not inconsequential. The bigger worry was, how this change is going to
effect servers with lots of devices connected to them.

For example, with this change, we are introducing a new uevent, which
was previously suppressed. Below is the udevadm monitor report which
shows the uevent generated when we define a new dasd device.

$: vmcp def t3390 e002 1
DASD E002 DEFINED
KERNEL[53.083552] add      /devices/css0/0.0.13af (css)
* KERNEL[53.083590] bind     /devices/css0/0.0.13af (css)
KERNEL[53.086561] add      /devices/css0/0.0.13af/0.0.e002 (ccw)
KERNEL[53.087136] bind     /devices/css0/0.0.13af/0.0.e002 (ccw)

This new uvent on css (Which is highlighed with *), does not have a bigger
impact on the machines. But, they are useless for those subchannels without
a proper device associated with it.

We wanted to make sure that this new uevents are not giving bigger
impacts on customer machines which would accommodate many more devices on
an LPAR. One test to simulate this scenario was to define more than 5000
dasd devices on a zVM and check the boot and initialization delay with and
without this patch. This does not show any impact on the zVM with high
number of devices.

I dont see any specific impact on this patch as of now. But, if you
think there is some more testing which are required before we push this
patch, please do let me know.


Vineeth Vijayan (1):
  s390/cio: remove uevent suppress from css driver

 drivers/s390/cio/chsc_sch.c     |  5 -----
 drivers/s390/cio/css.c          | 19 -------------------
 drivers/s390/cio/device.c       | 18 ------------------
 drivers/s390/cio/eadm_sch.c     |  5 -----
 drivers/s390/cio/vfio_ccw_drv.c |  5 -----
 5 files changed, 52 deletions(-)

-- 
2.25.1

[PATCH] s390/cio: remove uevent suppress from cio driver

[Vineeth Vijayan]

'commit fa1a8c23eb7d ("s390: cio: Delay uevents for subchannels")'
introduced the uevent suppression of subchannels. Even though there
are reasons for wanting to delay the uevent, it also introduces
problems. As part of cleaning-up the css driver,this patch removes
the uevent-suppress logic. The ADD uevent will be generated when
there is a valid subchannel and not after finding the associated
device. Removing the uevent suppress logic also introduces a new BIND
uevent associated to the channel-subsystem.

Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
---
 drivers/s390/cio/chsc_sch.c     |  5 -----
 drivers/s390/cio/css.c          | 19 -------------------
 drivers/s390/cio/device.c       | 18 ------------------
 drivers/s390/cio/eadm_sch.c     |  5 -----
 drivers/s390/cio/vfio_ccw_drv.c |  5 -----
 5 files changed, 52 deletions(-)

diff --git a/drivers/s390/cio/chsc_sch.c b/drivers/s390/cio/chsc_sch.c
index 684348d82f08..962dfa25a310 100644
--- a/drivers/s390/cio/chsc_sch.c
+++ b/drivers/s390/cio/chsc_sch.c
@@ -91,11 +91,6 @@ static int chsc_subchannel_probe(struct subchannel *sch)
 			 sch->schid.ssid, sch->schid.sch_no, ret);
 		dev_set_drvdata(&sch->dev, NULL);
 		kfree(private);
-	} else {
-		if (dev_get_uevent_suppress(&sch->dev)) {
-			dev_set_uevent_suppress(&sch->dev, 0);
-			kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-		}
 	}
 	return ret;
 }
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
index 2bc55ccf3f23..9638697b17de 100644
--- a/drivers/s390/cio/css.c
+++ b/drivers/s390/cio/css.c
@@ -470,16 +470,6 @@ int css_register_subchannel(struct subchannel *sch)
 	if (sch->st == SUBCHANNEL_TYPE_IO)
 		sch->dev.type = &io_subchannel_type;
 
-	/*
-	 * We don't want to generate uevents for I/O subchannels that don't
-	 * have a working ccw device behind them since they will be
-	 * unregistered before they can be used anyway, so we delay the add
-	 * uevent until after device recognition was successful.
-	 * Note that we suppress the uevent for all subchannel types;
-	 * the subchannel driver can decide itself when it wants to inform
-	 * userspace of its existence.
-	 */
-	dev_set_uevent_suppress(&sch->dev, 1);
 	css_update_ssd_info(sch);
 	/* make it known to the system */
 	ret = css_sch_device_register(sch);
@@ -488,15 +478,6 @@ int css_register_subchannel(struct subchannel *sch)
 			      sch->schid.ssid, sch->schid.sch_no, ret);
 		return ret;
 	}
-	if (!sch->driver) {
-		/*
-		 * No driver matched. Generate the uevent now so that
-		 * a fitting driver module may be loaded based on the
-		 * modalias.
-		 */
-		dev_set_uevent_suppress(&sch->dev, 0);
-		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-	}
 	return ret;
 }
 
diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
index 07a17613fab5..74e7d652d34f 100644
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -838,14 +838,6 @@ static void io_subchannel_register(struct ccw_device *cdev)
 		adjust_init_count = 0;
 		goto out;
 	}
-	/*
-	 * Now we know this subchannel will stay, we can throw
-	 * our delayed uevent.
-	 */
-	if (dev_get_uevent_suppress(&sch->dev)) {
-		dev_set_uevent_suppress(&sch->dev, 0);
-		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-	}
 	/* make it known to the system */
 	ret = device_add(&cdev->dev);
 	if (ret) {
@@ -1035,16 +1027,6 @@ static int io_subchannel_probe(struct subchannel *sch)
 				      "attributes for subchannel "
 				      "0.%x.%04x (rc=%d)\n",
 				      sch->schid.ssid, sch->schid.sch_no, rc);
-		/*
-		 * The console subchannel already has an associated ccw_device.
-		 * Throw the delayed uevent for the subchannel, register
-		 * the ccw_device and exit.
-		 */
-		if (dev_get_uevent_suppress(&sch->dev)) {
-			/* should always be the case for the console */
-			dev_set_uevent_suppress(&sch->dev, 0);
-			kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-		}
 		cdev = sch_get_cdev(sch);
 		rc = device_add(&cdev->dev);
 		if (rc) {
diff --git a/drivers/s390/cio/eadm_sch.c b/drivers/s390/cio/eadm_sch.c
index 15bdae5981ca..8b463681a149 100644
--- a/drivers/s390/cio/eadm_sch.c
+++ b/drivers/s390/cio/eadm_sch.c
@@ -243,11 +243,6 @@ static int eadm_subchannel_probe(struct subchannel *sch)
 	spin_lock_irq(&list_lock);
 	list_add(&private->head, &eadm_list);
 	spin_unlock_irq(&list_lock);
-
-	if (dev_get_uevent_suppress(&sch->dev)) {
-		dev_set_uevent_suppress(&sch->dev, 0);
-		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-	}
 out:
 	return ret;
 }
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 76099bcb765b..1d4bd852cb78 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -214,11 +214,6 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	if (ret)
 		goto out_disable;
 
-	if (dev_get_uevent_suppress(&sch->dev)) {
-		dev_set_uevent_suppress(&sch->dev, 0);
-		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-	}
-
 	VFIO_CCW_MSG_EVENT(4, "bound to subchannel %x.%x.%04x\n",
 			   sch->schid.cssid, sch->schid.ssid,
 			   sch->schid.sch_no);
-- 
2.25.1

Re: [PATCH] s390/cio: remove uevent suppress from cio driver

[Cornelia Huck]

On Wed, Oct 27 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:

> 'commit fa1a8c23eb7d ("s390: cio: Delay uevents for subchannels")'
> introduced the uevent suppression of subchannels. Even though there
> are reasons for wanting to delay the uevent, it also introduces
> problems. As part of cleaning-up the css driver,this patch removes
> the uevent-suppress logic. The ADD uevent will be generated when
> there is a valid subchannel and not after finding the associated
> device. Removing the uevent suppress logic also introduces a new BIND
> uevent associated to the channel-subsystem.

Hm, I'd probably put that a bit differently:

commit fa1a8c23eb7d ("s390: cio: Delay uevents for subchannels")
introduced suppression of uevents for a subchannel until after it is
clear that the subchannel would not be unregistered again
immediately. This was done to avoid uevents being generated for I/O
subchannels with no valid device, which can happen on LPAR.

However, this also has some drawbacks: All subchannel drivers need to
manually remove the uevent suppression and generate an ADD uevent as
soon as they are sure that the subchannel will stay around. This misses
out on all uevents that are not the initial ADD uevent that would be
generated while uevents are suppressed; for example, all subchannels
were missing the BIND uevent.

As uevents being generated even for I/O subchannels without an
operational device turned out to be not as bad as missing uevents and
complicating the code flow, let's remove uevent suppression for
subchannels.

>
> Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
> ---
>  drivers/s390/cio/chsc_sch.c     |  5 -----
>  drivers/s390/cio/css.c          | 19 -------------------
>  drivers/s390/cio/device.c       | 18 ------------------
>  drivers/s390/cio/eadm_sch.c     |  5 -----
>  drivers/s390/cio/vfio_ccw_drv.c |  5 -----
>  5 files changed, 52 deletions(-)

Patch looks sane to me, but I'll have to rely on your testing :)

Re: [RFC v2 0/1]s390/cio: remove uevent suppress from cio driver

[Cornelia Huck]

On Wed, Oct 27 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:

> This is the follow-up for the old RFC which i have posted here couple of
> months back. During the previous discussions on that RFC we concluded
> that removing the uevent-suppress from the CIO layer is the cleaner way
> of doing it and we should proceed. Reason for this RFC is, i want to
> convince myself that, i am not doing something wrong. I would like to
> bring up some of the tests i have done and the conclusion from those
> tests.
>
> The reason for introducing the delay in uevent generation was to avoid a
> uevent storm from those subchannels which does not have a valid device
> connected on this. I think for the new generation Z machines, this is
> not inconsequential. The bigger worry was, how this change is going to
> effect servers with lots of devices connected to them.
>
> For example, with this change, we are introducing a new uevent, which
> was previously suppressed. Below is the udevadm monitor report which
> shows the uevent generated when we define a new dasd device.
>
> $: vmcp def t3390 e002 1
> DASD E002 DEFINED
> KERNEL[53.083552] add      /devices/css0/0.0.13af (css)
> * KERNEL[53.083590] bind     /devices/css0/0.0.13af (css)
> KERNEL[53.086561] add      /devices/css0/0.0.13af/0.0.e002 (ccw)
> KERNEL[53.087136] bind     /devices/css0/0.0.13af/0.0.e002 (ccw)
>
> This new uvent on css (Which is highlighed with *), does not have a bigger
> impact on the machines. But, they are useless for those subchannels without
> a proper device associated with it.

Well, it's not exactly useless -- userspace gets notified that the
device has bound to a driver, which was an information that was
completely missing up to now for subchannels. The main issue is that the
subchannel will get deregistered again (or has that been changed? Sorry,
I've been out of the loop for too long...)

>
> We wanted to make sure that this new uevents are not giving bigger
> impacts on customer machines which would accommodate many more devices on
> an LPAR. One test to simulate this scenario was to define more than 5000
> dasd devices on a zVM and check the boot and initialization delay with and
> without this patch. This does not show any impact on the zVM with high
> number of devices.

I think the potentially problematic case is "lots of I/O subchannels
with no valid device", and I think you can't get that under z/VM (which
will not give you those subchannels in the first place), but only on LPAR.

>
> I dont see any specific impact on this patch as of now. But, if you
> think there is some more testing which are required before we push this
> patch, please do let me know.

I would probably also verify:
- non-I/O subchannels (IIRC you can't have CHSC subchannels under z/VM,
  don't know about EADM, so that would need to be done on an LPAR as
  well)
- interaction with driverctl (and maybe mdevctl) for vfio-ccw

But I think I also may be a bit out of touch here :)

>
>
> Vineeth Vijayan (1):
>   s390/cio: remove uevent suppress from css driver
>
>  drivers/s390/cio/chsc_sch.c     |  5 -----
>  drivers/s390/cio/css.c          | 19 -------------------
>  drivers/s390/cio/device.c       | 18 ------------------
>  drivers/s390/cio/eadm_sch.c     |  5 -----
>  drivers/s390/cio/vfio_ccw_drv.c |  5 -----
>  5 files changed, 52 deletions(-)

Re: [RFC v2 0/1]s390/cio: remove uevent suppress from cio driver

[Vineeth Vijayan]

Thank you Conny for looking in to this.

On Tue, 2021-11-02 at 16:31 +0100, Cornelia Huck wrote:

...snip...
> device has bound to a driver, which was an information that was
> completely missing up to now for subchannels. The main issue is that the
> subchannel will get deregistered again (or has that been changed? Sorry,
> I've been out of the loop for too long...)
You are right. There is a change and we do not unregister the subchannel if we
find the corresponding device is not operational or empty.

...snip...
> > I think the potentially problematic case is "lots of I/O subchannels
> with no valid device", and I think you can't get that under z/VM (which
> will not give you those subchannels in the first place), but only on LPAR.
Yes. But, this is in case of actual device. I just defined around 5k virtual
dasd devices on zVM and did not enable them. i.e those subchannels are not
blacklisted anymore, but they does not have an operational device. 

other than zVM, other than testing this patch on different available LPARs,
we tried to mimic the device availability with a custom test-kernel.
Basically, modified the subchannel initialization code and inform the subchannel
about the presence of a device, which then later fails during SNSID.
It is a horrid way to test it, but i think it could simulate some LPARs with
more than 1000 non-operational devices connected on it.

...snip...

> - non-I/O subchannels (IIRC you can't have CHSC subchannels under z/VM,
>   don't know about EADM, so that would need to be done on an LPAR as
>   well)
Thanks. Most of my tests were on IO-subchannel. I shall try few tests on CHSC
and EADM.

> - interaction with driverctl (and maybe mdevctl) for vfio-ccw
I have done couple of tests with driverctl. Also, i was wondering if vfio-ccw
can make use of 'dev_busid' sysfs-interface under each subchannels while writing
the rules. This is totally different topic, which i do not want to mix up in
this thread.

...snip...

Re: [RFC v2 0/1]s390/cio: remove uevent suppress from cio driver

[Cornelia Huck]

On Wed, Nov 03 2021, Vineeth Vijayan <vneethv@linux.ibm.com> wrote:

>> > I think the potentially problematic case is "lots of I/O subchannels
>> with no valid device", and I think you can't get that under z/VM (which
>> will not give you those subchannels in the first place), but only on LPAR.
> Yes. But, this is in case of actual device. I just defined around 5k virtual
> dasd devices on zVM and did not enable them. i.e those subchannels are not
> blacklisted anymore, but they does not have an operational device. 
>
> other than zVM, other than testing this patch on different available LPARs,
> we tried to mimic the device availability with a custom test-kernel.
> Basically, modified the subchannel initialization code and inform the subchannel
> about the presence of a device, which then later fails during SNSID.
> It is a horrid way to test it, but i think it could simulate some LPARs with
> more than 1000 non-operational devices connected on it.

OK, that should be a way to figure out how userspace deals with the
extra uevents.

>
> ...snip...
>
>> - non-I/O subchannels (IIRC you can't have CHSC subchannels under z/VM,
>>   don't know about EADM, so that would need to be done on an LPAR as
>>   well)
> Thanks. Most of my tests were on IO-subchannel. I shall try few tests on CHSC
> and EADM.
>
>> - interaction with driverctl (and maybe mdevctl) for vfio-ccw
> I have done couple of tests with driverctl. Also, i was wondering if vfio-ccw
> can make use of 'dev_busid' sysfs-interface under each subchannels while writing
> the rules. This is totally different topic, which i do not want to mix up in
> this thread.

Oh, did not know about that interface.

<looks>
Doesn't the code need to check for 'w' for message subchannels, though?

'dev_busid' looks like a good fit for udev rules; maybe driverctl needs
special-casing? Or does it belong into mdevctl? cc:ing vfio-ccw
maintainers for awareness :)

Re: [RFC v2 0/1]s390/cio: remove uevent suppress from cio driver

[Vineeth Vijayan]

On Wed, 2021-11-03 at 16:41 +0100, Cornelia Huck wrote:
> > > - interaction with driverctl (and maybe mdevctl) for vfio-ccw
> > I have done couple of tests with driverctl. Also, i was wondering if vfio-ccw
> > can make use of 'dev_busid' sysfs-interface under each subchannels while writing
> > the rules. This is totally different topic, which i do not want to mix up in
> > this thread.
> 
> Oh, did not know about that interface.
> 
> <looks>
> Doesn't the code need to check for 'w' for message subchannels, though?
You are right. For the message-subchannels, we need to check 'W'.
I will send a fix.

Thank you.