[PATCH] test/cxl-xor-region.sh: remove redundant waitting

[PATCH] test/cxl-xor-region.sh: remove redundant waitting

[ruansy.fnst]

From: Ruan Shiyang <ruansy.fnst@fujitsu.com>

Now that cxl_wait_probe() has been added[1] to wait for udev queue
empty, the `udevadm settle` here is no longer necessary.

[1] b231603 cxl/lib: Add cxl_wait_probe()

Signed-off-by: Ruan Shiyang <ruansy.fnst@fujitsu.com>

===
Question to Dan:

I understand how cxl_wait_probe() work, but I have some questions about
the motivation of adding this function:  Firstly, is it function added
for simply waiting for new added CXL device been ready before cxl
command does the actual work?  Just for replacing `udevadm settle`'s
work?

Now I am facing a problem that cxl command takes a long time to complete
when I run it in a udev rule(do some configuration when CXL memdev is
added).  I found it is caused by this function: waitting for udev
queue's endding but itself is in the queue.  The cxl_wait_probe()
function does not seem to allow me to do that.  So, the 2nd question is:
is it against the spec to run cxl command in a udev rule?
====
---
 test/cxl-xor-region.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/test/cxl-xor-region.sh b/test/cxl-xor-region.sh
index b9e1d79..fb4f9a0 100644
--- a/test/cxl-xor-region.sh
+++ b/test/cxl-xor-region.sh
@@ -14,7 +14,6 @@ check_prereq "jq"
 
 modprobe -r cxl_test
 modprobe cxl_test interleave_arithmetic=1
-udevadm settle
 rc=1
 
 # THEORY OF OPERATION: Create x1,2,3,4 regions to exercise the XOR math
-- 
2.43.0

Re: [PATCH] test/cxl-xor-region.sh: remove redundant waitting

[Alison Schofield]

On Wed, May 14, 2025 at 07:20:03PM +0800, ruansy.fnst wrote:
> From: Ruan Shiyang <ruansy.fnst@fujitsu.com>
> 
> Now that cxl_wait_probe() has been added[1] to wait for udev queue
> empty, the `udevadm settle` here is no longer necessary.
> 
> [1] b231603 cxl/lib: Add cxl_wait_probe()
> 
> Signed-off-by: Ruan Shiyang <ruansy.fnst@fujitsu.com>

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Hi Ruan, It looks like I snuck this one in right before Dan introduced
udevadm settle and cleaned up all the usages. I'll take this patch as
is. The next time you patch ndctl, do these:

[PATCH] --> [ndctl PATCH]
Send to nvdimm@lists.linux.dev and 'cc linux-cxl@vger.kernel.org
if it's CXL related like this one.

I suggest you resend this udev question in a new email to linux-cxl
list to draw attention. 

Thanks for the patch!

> 
> ===
> Question to Dan:
> 
> I understand how cxl_wait_probe() work, but I have some questions about
> the motivation of adding this function:  Firstly, is it function added
> for simply waiting for new added CXL device been ready before cxl
> command does the actual work?  Just for replacing `udevadm settle`'s
> work?
> 
> Now I am facing a problem that cxl command takes a long time to complete
> when I run it in a udev rule(do some configuration when CXL memdev is
> added).  I found it is caused by this function: waitting for udev
> queue's endding but itself is in the queue.  The cxl_wait_probe()
> function does not seem to allow me to do that.  So, the 2nd question is:
> is it against the spec to run cxl command in a udev rule?
> ====
> ---
>  test/cxl-xor-region.sh | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/test/cxl-xor-region.sh b/test/cxl-xor-region.sh
> index b9e1d79..fb4f9a0 100644
> --- a/test/cxl-xor-region.sh
> +++ b/test/cxl-xor-region.sh
> @@ -14,7 +14,6 @@ check_prereq "jq"
>  
>  modprobe -r cxl_test
>  modprobe cxl_test interleave_arithmetic=1
> -udevadm settle
>  rc=1
>  
>  # THEORY OF OPERATION: Create x1,2,3,4 regions to exercise the XOR math
> -- 
> 2.43.0
> 
> 

Re: [PATCH] test/cxl-xor-region.sh: remove redundant waitting

[Shiyang Ruan]

在 2025/5/17 5:36, Alison Schofield 写道:
> On Wed, May 14, 2025 at 07:20:03PM +0800, ruansy.fnst wrote:
>> From: Ruan Shiyang <ruansy.fnst@fujitsu.com>
>>
>> Now that cxl_wait_probe() has been added[1] to wait for udev queue
>> empty, the `udevadm settle` here is no longer necessary.
>>
>> [1] b231603 cxl/lib: Add cxl_wait_probe()
>>
>> Signed-off-by: Ruan Shiyang <ruansy.fnst@fujitsu.com>
> 
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> 
> Hi Ruan, It looks like I snuck this one in right before Dan introduced
> udevadm settle and cleaned up all the usages. I'll take this patch as
> is. The next time you patch ndctl, do these:
> 
> [PATCH] --> [ndctl PATCH]
> Send to nvdimm@lists.linux.dev and 'cc linux-cxl@vger.kernel.org
> if it's CXL related like this one.

OK, got it.  I'll add 'ndctl' tag and cc nvdimm&cxl ml next time. 
Thanks for pointing out.

> 
> I suggest you resend this udev question in a new email to linux-cxl
> list to draw attention.

OK.  The lucky thing is that they have noticed this and started discussion.

> 
> Thanks for the patch!

--
Thanks,
Ruan.

> 
>>
>> ===
>> Question to Dan:
>>
>> I understand how cxl_wait_probe() work, but I have some questions about
>> the motivation of adding this function:  Firstly, is it function added
>> for simply waiting for new added CXL device been ready before cxl
>> command does the actual work?  Just for replacing `udevadm settle`'s
>> work?
>>
>> Now I am facing a problem that cxl command takes a long time to complete
>> when I run it in a udev rule(do some configuration when CXL memdev is
>> added).  I found it is caused by this function: waitting for udev
>> queue's endding but itself is in the queue.  The cxl_wait_probe()
>> function does not seem to allow me to do that.  So, the 2nd question is:
>> is it against the spec to run cxl command in a udev rule?
>> ====
>> ---
>>   test/cxl-xor-region.sh | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/test/cxl-xor-region.sh b/test/cxl-xor-region.sh
>> index b9e1d79..fb4f9a0 100644
>> --- a/test/cxl-xor-region.sh
>> +++ b/test/cxl-xor-region.sh
>> @@ -14,7 +14,6 @@ check_prereq "jq"
>>   
>>   modprobe -r cxl_test
>>   modprobe cxl_test interleave_arithmetic=1
>> -udevadm settle
>>   rc=1
>>   
>>   # THEORY OF OPERATION: Create x1,2,3,4 regions to exercise the XOR math
>> -- 
>> 2.43.0
>>
>>

cxl <-> udev deadlock?

[Marc Herbert]

On 2025-05-14 04:20, ruansy.fnst wrote:
> 
> Now that cxl_wait_probe() has been added[1] to wait for udev queue
> empty, the `udevadm settle` here is no longer necessary.
> 
> [1] b231603 cxl/lib: Add cxl_wait_probe()
> 
> Signed-off-by: Ruan Shiyang <ruansy.fnst@fujitsu.com>
> 
> ===
> Question to Dan:
> 
> I understand how cxl_wait_probe() work, but I have some questions about
> the motivation of adding this function:  Firstly, is it function added
> for simply waiting for new added CXL device been ready before cxl
> command does the actual work?  Just for replacing `udevadm settle`'s
> work?
> 
> Now I am facing a problem that cxl command takes a long time to complete
> when I run it in a udev rule(do some configuration when CXL memdev is
> added).  I found it is caused by this function: waitting for udev
> queue's endding but itself is in the queue.  The cxl_wait_probe()
> function does not seem to allow me to do that.  So, the 2nd question is:
> is it against the spec to run cxl command in a udev rule?

cxl waits for udev which waits for cxl... this looks like an interesting
deadlock!?

When you write "a long time to complete", do you mean "aborted after the
default, 180s udev timeout?"

https://www.freedesktop.org/software/systemd/man/latest/systemd-udevd.service.html

udev is not designed to start long-running processes BUT udev is designed
to communicate with systemd which is the correct manager for all
long-running processes:

https://www.freedesktop.org/software/systemd/man/latest/systemd.device.html

You can find examples on your system like this:

  grep -r SYSTEMD_.*WANT /usr/lib/udev/

Now, I think you are not interested in running anything for _long_. But,
the same technique and indirection should also break the deadlock (if any)
because udev messages to systemd are "fire and forget".

Also, systemd is designed to collect and report logs, exit status
etc. which is very useful when something goes wrong. udev does not
do that.

Re: [PATCH] test/cxl-xor-region.sh: remove redundant waitting

[dan.j.williams]

ruansy.fnst wrote:
> From: Ruan Shiyang <ruansy.fnst@fujitsu.com>
> 
> Now that cxl_wait_probe() has been added[1] to wait for udev queue
> empty, the `udevadm settle` here is no longer necessary.
> 
> [1] b231603 cxl/lib: Add cxl_wait_probe()
> 
> Signed-off-by: Ruan Shiyang <ruansy.fnst@fujitsu.com>
> 
> ===
> Question to Dan:
> 
> I understand how cxl_wait_probe() work, but I have some questions about
> the motivation of adding this function:  Firstly, is it function added
> for simply waiting for new added CXL device been ready before cxl
> command does the actual work?  Just for replacing `udevadm settle`'s
> work?

No the motivation is that a command like "cxl list" wants to be to
determine that a given object is "enabled", i.e. driver attached. At any
given point of the time the cxl_pci driver could be going through async
attach, and the arrival of the cxl_acpi driver may trigger
cxl_bus_rescan().

So the assumption is that interfactive invocation of cxl commands should
arrange for a stablized snaphshot of the topology state.

> Now I am facing a problem that cxl command takes a long time to complete
> when I run it in a udev rule(do some configuration when CXL memdev is
> added).  I found it is caused by this function: waitting for udev
> queue's endding but itself is in the queue.  The cxl_wait_probe()
> function does not seem to allow me to do that.

Yes, that is a problem.

> So, the 2nd question is: is it against the spec to run cxl command in
> a udev rule?

No we need to find a way to make that work, so I consider this a bug
report. My initial reaction is that when called from a udev rule cxl-cli
should honor a new environment variable, perhaps "UDEV_CXL", that
disables this waiting that is only meant for human interactive mode.

The udev rule would need to know the potential asynchronous states of
the topology.  However, as long as it is performing object local
operations it should be fine. For example "UDEV_CXL=1; cxl list -m
$memdev", where $memdev just experienced the "add" uevent, should just
dump that object's details without waiting for the rest of the topology
to settle.

Re: [PATCH] test/cxl-xor-region.sh: remove redundant waitting

[Marc Herbert]

On 2025-05-16 17:23, dan.j.williams@intel.com wrote:

> 
> So the assumption is that interfactive invocation of cxl commands should
> arrange for a stablized snaphshot of the topology state.
> 
>> Now I am facing a problem that cxl command takes a long time to complete
>> when I run it in a udev rule(do some configuration when CXL memdev is
>> added).  I found it is caused by this function: waitting for udev
>> queue's endding but itself is in the queue.  The cxl_wait_probe()
>> function does not seem to allow me to do that.
> 
> Yes, that is a problem.
> 
>> So, the 2nd question is: is it against the spec to run cxl command in
>> a udev rule?
> 
> No we need to find a way to make that work, so I consider this a bug
> report. My initial reaction is that when called from a udev rule cxl-cli
> should honor a new environment variable, perhaps "UDEV_CXL", that
> disables this waiting that is only meant for human interactive mode.
> 

If the distinction between two such "modes" is indeed desired, I don't
think the name should point at the first known use case (UDEV) for
it. For instance, you could also imagine that while some parts of test
code want the current, blocking and "stabilized" behavior, other parts
of some (other) test code want the currently missing), "fast" and
unstable information without waiting. There could be other use cases, so
in general the user interface (if any) should describe what it does, not
who it is for.

Also, why not just a new command line option like cxl --no-wait?
Environment variables are convenient to cut through layers but they are
also invisible which tends to cause debugging nightmares.  Bonus point
when racing against something like else - which is exactly the case
here! I can already feel for the poor soul copying commands from the
logs of some test failure and missing the magic environment
variable(s)... been there, done that.

PS: while... waiting for such a fast/no-wait/unstable option, I see no
reason why systemd would not work.

Re: [PATCH] test/cxl-xor-region.sh: remove redundant waitting

[Dan Williams]

Marc Herbert wrote:
> On 2025-05-16 17:23, dan.j.williams@intel.com wrote:
> 
> > 
> > So the assumption is that interfactive invocation of cxl commands should
> > arrange for a stablized snaphshot of the topology state.
> > 
> >> Now I am facing a problem that cxl command takes a long time to complete
> >> when I run it in a udev rule(do some configuration when CXL memdev is
> >> added).  I found it is caused by this function: waitting for udev
> >> queue's endding but itself is in the queue.  The cxl_wait_probe()
> >> function does not seem to allow me to do that.
> > 
> > Yes, that is a problem.
> > 
> >> So, the 2nd question is: is it against the spec to run cxl command in
> >> a udev rule?
> > 
> > No we need to find a way to make that work, so I consider this a bug
> > report. My initial reaction is that when called from a udev rule cxl-cli
> > should honor a new environment variable, perhaps "UDEV_CXL", that
> > disables this waiting that is only meant for human interactive mode.
> > 
> 
> If the distinction between two such "modes" is indeed desired, I don't
> think the name should point at the first known use case (UDEV) for
> it. For instance, you could also imagine that while some parts of test
> code want the current, blocking and "stabilized" behavior, other parts
> of some (other) test code want the currently missing), "fast" and
> unstable information without waiting. There could be other use cases, so
> in general the user interface (if any) should describe what it does, not
> who it is for.

This can be a surgical UDEV specific environment option for now because
there are no other use cases identified. The command line arguments can
always be extended later if need be.

[..]
> PS: while... waiting for such a fast/no-wait/unstable option, I see no
> reason why systemd would not work.

It does not work due to circular dependency deadlock.

Re: [PATCH] test/cxl-xor-region.sh: remove redundant waitting

[Marc Herbert]

On 2025-05-19 16:09, Dan Williams wrote:
> Marc Herbert wrote:

>> PS: while... waiting for such a fast/no-wait/unstable option, I see no
>> reason why systemd would not work.
> 
> It does not work due to circular dependency deadlock.

I just had a quick chat with Dan about this.

A local, systemd indirection _workaround_ would avoid the
deadlock. systemd is dealing with processes than "hang" by design:
daemons! And with even more complex and difficult processes and
services. udev routinely sends messages and triggers systemd, yet
systemd has never blocked udev in return, no matter what happens.
When you connect a USB printer, udev and systemd start the ipp-usb
process which "hangs" forever. This does not block udev.

On the other hand, in
https://lore.kernel.org/linux-cxl/ef46edd7-b492-4124-9cf9-0db104175eac@linux.intel.com/
I never meant to suggest systemd as a "permanent" solution for every
user of the cxl command! I was only suggesting systemd as a "private"
workaround, part of some specific test suite currently stuck with this
deadlock.

Hope this clarifies.

Re: [PATCH] test/cxl-xor-region.sh: remove redundant waitting

[Shiyang Ruan]

在 2025/5/20 9:00, Marc Herbert 写道:
> 
> 
> On 2025-05-19 16:09, Dan Williams wrote:
>> Marc Herbert wrote:
> 
>>> PS: while... waiting for such a fast/no-wait/unstable option, I see no
>>> reason why systemd would not work.
>>
>> It does not work due to circular dependency deadlock.
> 
> I just had a quick chat with Dan about this.
> 
> A local, systemd indirection _workaround_ would avoid the
> deadlock. systemd is dealing with processes than "hang" by design:
> daemons! And with even more complex and difficult processes and
> services. udev routinely sends messages and triggers systemd, yet
> systemd has never blocked udev in return, no matter what happens.
> When you connect a USB printer, udev and systemd start the ipp-usb
> process which "hangs" forever. This does not block udev.
> 
> On the other hand, in
> https://lore.kernel.org/linux-cxl/ef46edd7-b492-4124-9cf9-0db104175eac@linux.intel.com/
> I never meant to suggest systemd as a "permanent" solution for every
> user of the cxl command! I was only suggesting systemd as a "private"
> workaround, part of some specific test suite currently stuck with this
> deadlock.
> 
> Hope this clarifies.

Thanks for the advice!  Yes, I have tried to use `systemd-run 
my-original-script.sh` to achieve my goal.  It creates a new process 
which is outside of udev queue so it won't cause the deadlock.

BTW, after you talked with Dan, do you think it is still necessary to 
add the `--no-wait` parameter to avoid waiting for the udev queue empty 
in some specific scenarios?  Do you have any plans to implement this 
feature?


--
Thanks,
Ruan.

Re: [PATCH] test/cxl-xor-region.sh: remove redundant waitting

[Marc Herbert]

On 2025-05-19 20:38, Shiyang Ruan wrote:
> 
> Yes, I have tried to use `systemd-run my-original-script.sh` to
> achieve my goal.  It creates a new process which is outside of udev
> queue so it won't cause the deadlock.

Thanks for confirming! Also, I didn't know about systemd-run, it looks
very useful. Thanks for sharing. run_qemu.sh should probably use it; it
would simplify things.

Learning about systemd is a lifelong process...

> BTW, after you talked with Dan, do you think it is still necessary to
> add the `--no-wait` parameter to avoid waiting for the udev queue
> empty in some specific scenarios?

Dan's recommendation to have two different "modes" still makes sense to
me.

> Do you have any plans to implement this feature?

I'm not aware of anyone who volunteered to do this yet. I bet
there will be reviewers if someone does. 

Re: [PATCH] test/cxl-xor-region.sh: remove redundant waitting

[Alison Schofield]

On Wed, May 14, 2025 at 07:20:03PM +0800, ruansy.fnst wrote:
> From: Ruan Shiyang <ruansy.fnst@fujitsu.com>
> 
> Now that cxl_wait_probe() has been added[1] to wait for udev queue
> empty, the `udevadm settle` here is no longer necessary.
> 
> [1] b231603 cxl/lib: Add cxl_wait_probe()
> 
> Signed-off-by: Ruan Shiyang <ruansy.fnst@fujitsu.com>

Applied to pending branch:
https://github.com/pmem/ndctl/tree/pending

> 
> ===
> Question to Dan:
> 
> I understand how cxl_wait_probe() work, but I have some questions about
> the motivation of adding this function:  Firstly, is it function added
> for simply waiting for new added CXL device been ready before cxl
> command does the actual work?  Just for replacing `udevadm settle`'s
> work?
> 
> Now I am facing a problem that cxl command takes a long time to complete
> when I run it in a udev rule(do some configuration when CXL memdev is
> added).  I found it is caused by this function: waitting for udev
> queue's endding but itself is in the queue.  The cxl_wait_probe()
> function does not seem to allow me to do that.  So, the 2nd question is:
> is it against the spec to run cxl command in a udev rule?
> ====
> ---
>  test/cxl-xor-region.sh | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/test/cxl-xor-region.sh b/test/cxl-xor-region.sh
> index b9e1d79..fb4f9a0 100644
> --- a/test/cxl-xor-region.sh
> +++ b/test/cxl-xor-region.sh
> @@ -14,7 +14,6 @@ check_prereq "jq"
>  
>  modprobe -r cxl_test
>  modprobe cxl_test interleave_arithmetic=1
> -udevadm settle
>  rc=1
>  
>  # THEORY OF OPERATION: Create x1,2,3,4 regions to exercise the XOR math
> -- 
> 2.43.0
> 
>