[satyamg1620]

Motivation

Add s390x support for taskdump

Solution

[satyamg1620]

@Darksonn @mattiapitossi Can you please review this PR?

[Darksonn]

Since we do not have the testing facilities to run tests on s390x, can you please confirm that you tested that this is working and passing tests?

Otherwise the change looks fine to me.

[martin-g]

Some more places need to be updated:

• tokio/examples/dump.rs

  Line 8 in 299a10d

  any(target_arch = "aarch64", target_arch = "x86", target_arch = "x86_64")

• tokio/examples/dump.rs

  Line 85 in 299a10d

  any(target_arch = "aarch64", target_arch = "x86", target_arch = "x86_64")

• tokio/tokio/tests/task_trace_self.rs

  Line 6 in 299a10d

  any(target_arch = "aarch64", target_arch = "x86", target_arch = "x86_64")

[martin-g]

Suggested change

	/// Task dumps are supported on Linux atop `aarch64`, `x86`, `x86_64` and `s390x`.

[satyamg1620]

I will update the required changes

[rcoh]

Maybe worth extracting a macro for task dumps to avoid all the duplicated spots?

[martin-g]

Maybe worth extracting a macro for task dumps to avoid all the duplicated spots?

There is such one - https://github.com/tokio-rs/tokio/pull/8192/changes#diff-a772eaa4d37eb6328d28e246aff280fc7074949158c13be8ad006f8218227b3aR509
@satyamg1620 Could you please check whether it could be applied to some of the places where it is not used now ? Thanks!

[satyamg1620]

ok @martin-g

[satyamg1620]

Since we do not have the testing facilities to run tests on s390x, can you please confirm that you tested that this is working and passing tests?

Otherwise the change looks fine to me.

Ok I will attach the test logs

[satyamg1620]

Since we do not have the testing facilities to run tests on s390x, can you please confirm that you tested that this is working and passing tests?
Otherwise the change looks fine to me.

Ok I will attach the test logs

test logs produced after running cargo test -p tokio --features full,test-util,taskdump,tracing -- --test-threads=1 --skip try_exists 2>&1 s390x-proof.log`

[martin-g]

test logs produced after running cargo test -p tokio --features full,test-util,taskdump,tracing -- --test-threads=1 --skip try_exists 2>&1 s390x-proof.log`

These results do not confirm that the taskdump functionality works.
From the logs I cannot say that the taskdump feature is enabled or not.
Better execute the examples/dump.rs example and show us its output.

[satyamg1620]

test logs produced after running cargo test -p tokio --features full,test-util,taskdump,tracing -- --test-threads=1 --skip try_exists 2>&1 s390x-proof.log`

These results do not confirm that the taskdump functionality works. From the logs I cannot say that the taskdump feature is enabled or not. Better execute the examples/dump.rs example and show us its output.

Hi @martin-g , following is output for execution of examples/dump.rs
s390x-dump-output.log

Let me know if any other thing to be provided

[martin-g]

We can also "compress" this to: #[cfg(taskdump)] by adding a build.rs file with content like:

fn main() {
    #[cfg(all(
        tokio_unstable,
        feature = "taskdump",
        feature = "rt",
        target_os = "linux",
        any(
            target_arch = "aarch64",
            target_arch = "x86",
            target_arch = "x86_64",
            target_arch = "s390x"
        )
    ))]
    println!("cargo:rustc-cfg=taskdump");
}

And it could be reused at several other places.

Do we want to do it ?

[satyamg1620]

ok @martin-g . I will do it

[martin-g]

@satyamg1620 Let's wait for some more opinions from the Tokio team!
We could do it in a follow-up too!

[martin-g]

Or you could do it now to show how it will improve the current code but we may need to revert the commit if someone disagrees with the idea. As you prefer!

[satyamg1620]

I will try to do it. We can also ask @Darksonn @rcoh regarding this for some suggestions.

[Darksonn]

Let's not add a build.rs just for this.

[satyamg1620]

@Darksonn so should I put this commit 9213ae1 as it is or revert it?

[Darksonn]

That commit is fine.

[satyamg1620]

If it is ok, can you merge the PR?

[satyamg1620]

@Darksonn @rcoh, can you please review the new changes? If they are ok, can you please merge the PR, if possible

[satyamg1620]

@Darksonn @rcoh @martin-g , can you please merge the PR, if it looks good

[Darksonn]

Thanks for the PR!

[satyamg1620]

Thanks @Darksonn