test: A test program for the new mwait implementation.

Information
Submitter:	Derek Hower
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9467:84decd4c3a2d
---------------------------
test: A test program for the new mwait implementation.

This is a simple test program for the new mwait implemenation. It is uses
m5threads to create to threads of execution in syscall emulation mode that
interact using the mwait instruction.

*Note* This patch was originally created by Marc Orr (morr@cs.wisc.edu)

Testing Done

Issue Summary

Description	From	Last Updated	Status
This is not used anywhere.	Nilay Vaish	Jan. 25, 2013, 1:23 a.m.	Open
It seems you are only using flags[0] and wait[0]. Why create space for 10?	Nilay Vaish	Jan. 25, 2013, 1:23 a.m.	Open
Since the code is being copied from the linux kernel, would it not come under GPL?	Nilay Vaish	Jan. 25, 2013, 1:23 a.m.	Open
How do you know this would run on a different cpu?	Nilay Vaish	Jan. 25, 2013, 1:23 a.m.	Open
Are sure that pthread.o is available?	Nilay Vaish	Feb. 8, 2013, 8:09 a.m.	Open
Why do have Makefile in the dependency list? Had it been the 'all' target which depends on the Makefile, that ...	Nilay Vaish	Feb. 8, 2013, 8:09 a.m.	Open
These constants depend on the the cache line size, as you explained before. I think it would be good if ...	Nilay Vaish	Feb. 8, 2013, 8:09 a.m.	Open
This argument is not in use anywhere.	Nilay Vaish	Feb. 8, 2013, 8:09 a.m.	Open
Normally CXX and should not even have to be defined in the Makefile (implicit)	Andreas Hansson	March 4, 2013, 5:43 a.m.	Open
The %.o: %.c is an implicit rule. All you have to do is override CPPFLAGS or CXXFLAGS as appropriate	Andreas Hansson	March 4, 2013, 5:43 a.m.	Open

Description:

		Changeset 9467:84decd4c3a2d

		test: A test program for the new mwait implementation.

		This is a simple test program for the new mwait implemenation. It is uses
		m5threads to create to threads of execution in syscall emulation mode that
		interact using the mwait instruction.

~		Note This patch was originally created by Marc Orr
	~	Note This patch was originally created by Marc Orr (morr@cs.wisc.edu)

Marc Orr Jan. 25, 2013, 4:21 a.m. (Jan. 25, 2013, 4:21 a.m.)
```
Hi Nilay,

Thanks for the comments!

Marc
```

tests/test-progs/mwait/mwait.c (Diff revision 1)

This is not used anywhere.

Show all issues

Marc Orr Jan. 25, 2013, 4:21 a.m. (Jan. 25, 2013, 4:21 a.m.)
```
Fixed in a new revision that I sent Derek.
```

tests/test-progs/mwait/mwait.c (Diff revision 1)

It seems you are only using flags[0] and wait[0]. Why create space for 10?

Show all issues

Marc Orr Jan. 25, 2013, 4:21 a.m. (Jan. 25, 2013, 4:21 a.m.)

To ensure that flags and wait don't sit in the same cache line.

tests/test-progs/mwait/mwait.c (Diff revision 1)

Since the code is being copied from the linux kernel, would it not come under GPL?

Show all issues

Marc Orr Jan. 25, 2013, 4:21 a.m. (Jan. 25, 2013, 4:21 a.m.)

I changed the code to call mwait and monitor via gcc intrinsics in a new revision that I sent to Derek.

tests/test-progs/mwait/mwait.c (Diff revision 1)

How do you know this would run on a different cpu?

Show all issues

Marc Orr Jan. 25, 2013, 4:21 a.m. (Jan. 25, 2013, 4:21 a.m.)

I've added code to make sure that thread 0 is schedueld on core 0 and thread 1 is scheduled on core 1 in a new revision that I sent to Derek.

Change Summary:

Updates from Marc.

Diff:

Revision 2 (+131)

Show changes

	tests/test-progs/mwait/Makefile
	tests/test-progs/mwait/mwait.c

Diff:

Revision 3 (+132)

Show changes

	tests/test-progs/mwait/Makefile
	tests/test-progs/mwait/mwait.c

Marc Orr Feb. 16, 2013, 6:27 a.m. (Feb. 16, 2013, 6:27 a.m.)
```
Thanks for the comments.
```

tests/test-progs/mwait/Makefile (Diff revision 3)

Extra space at the end of line.

tests/test-progs/mwait/Makefile (Diff revision 3)

Are sure that pthread.o is available?

Show all issues

Marc Orr Feb. 16, 2013, 6:27 a.m. (Feb. 16, 2013, 6:27 a.m.)

Well, it won't be available. This patch adds the object file to the repo. I don't think this is a good precedent to set though.

Maybe instead, I can add a README file that explains how to get m5threads, and built the pthread.o file? How does this sound? do you have any other suggestions?

Marc Orr March 4, 2013, 4:48 a.m. (March 4, 2013, 4:48 a.m.)

Nilay, are you ok with a README file here? Once I have clear direction on this comment, I will submit a new version of this patch.

Nilay Vaish March 4, 2013, 6:46 a.m. (March 4, 2013, 6:46 a.m.)

While we do have some object files in the repo, but those are only for the
Hello World program. I don't know how source files and binaries for all the 
other tests are maintained. We should go the same way for this binary as 
well. You should ping Steve on this issue.

tests/test-progs/mwait/Makefile (Diff revision 3)

Why do have Makefile in the dependency list? Had it been the 'all' target which depends on the Makefile, that would look fine. But the .o file should only depend on the .c file.

Show all issues

Marc Orr Feb. 16, 2013, 6:27 a.m. (Feb. 16, 2013, 6:27 a.m.)

Hrmmm, I'd never heard that its not ok for a object file to depend on a Makefile. I think I was editing the Makefile a lot, so this was a convenient dependence. I'm not particularly attached to this dependency though, so I can remove it.

tests/test-progs/mwait/mwait.c (Diff revision 3)

These constants depend on the the cache line size, as you explained before. I think it would be good if you reintroduce that #define you had earlier (or may be a const variable) that is named cache line size and use that in place of 10.

Show all issues

tests/test-progs/mwait/mwait.c (Diff revision 3)

This argument is not in use anywhere.

Show all issues

tests/test-progs/mwait/Makefile (Diff revision 3)

Normally CXX and should not even have to be defined in the Makefile (implicit)

Show all issues

tests/test-progs/mwait/Makefile (Diff revision 3)

The %.o: %.c is an implicit rule. All you have to do is override CPPFLAGS or CXXFLAGS as appropriate

Show all issues

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 9467:84decd4c3a2d

Change Summary:

Diff:

Diff:

Status: Closed (submitted)