Ns-3 MPI Distributed Simulation

[faker.moatamri, 2009-11-06 14:22:30]

http://codereview.appspot.com/109068/diff/2034/2035
File examples/mpi/Makefile (right):

http://codereview.appspot.com/109068/diff/2034/2035#newcode4
examples/mpi/Makefile:4: # Change the NS3BASE below to the path of your base
ns-3
is your code compatible with the waf system?

http://codereview.appspot.com/109068/diff/2034/2036
File examples/mpi/README (right):

http://codereview.appspot.com/109068/diff/2034/2036#newcode23
examples/mpi/README:23: 4) Build ns-3 with the --with-mpi option
it would be nice that this command also builds the examples without having to go
through the following steps.

http://codereview.appspot.com/109068/diff/2034/2037
File examples/mpi/nms-udp-nix.cc (right):

http://codereview.appspot.com/109068/diff/2034/2037#newcode1
examples/mpi/nms-udp-nix.cc:1: // DARPA NMS Campus Network Model
Files should begin with the licence

http://codereview.appspot.com/109068/diff/2034/2037#newcode5
examples/mpi/nms-udp-nix.cc:5: // - Link Bandwidths and Delays may not be the
same as the original
more details about the model implemented would be cool, for example a network
topology scheme.

http://codereview.appspot.com/109068/diff/2034/2037#newcode41
examples/mpi/nms-udp-nix.cc:41: main (int argc, char *argv[])
around 500 lines for the main, I would recommend using functions/methods with
names so that the code is more modular

http://codereview.appspot.com/109068/diff/2034/2037#newcode51
examples/mpi/nms-udp-nix.cc:51: //RandomVariable::UseGlobalSeed (1, 1, 2, 3, 5,
8);
remove

http://codereview.appspot.com/109068/diff/2034/2037#newcode56
examples/mpi/nms-udp-nix.cc:56: uint32_t rank = MPIInterface::Rank();;
I know it's harmless but please remove the second semi-column

http://codereview.appspot.com/109068/diff/2034/2037#newcode83
examples/mpi/nms-udp-nix.cc:83: 
I think a topology of the network is more than necessary here

http://codereview.appspot.com/109068/diff/2034/2037#newcode123
examples/mpi/nms-udp-nix.cc:123: nodes_net0[z][0].Add(nodes_net0[z][1].Get(0));
Coding style, mainly space before parentheses, also quotes, for example it
should be 
for ()
  {
    //code
  }
for all your files please, you can take a look at coding style page on nsnam.org

http://codereview.appspot.com/109068/diff/2034/2038
File examples/mpi/test-distributed.cc (right):

http://codereview.appspot.com/109068/diff/2034/2038#newcode58
examples/mpi/test-distributed.cc:58: main (int argc, char *argv[])
I would also recommand here to modularize this function.

http://codereview.appspot.com/109068/diff/2034/2039
File src/common/buffer.cc (right):

http://codereview.appspot.com/109068/diff/2034/2039#newcode661
src/common/buffer.cc:661: Buffer tmp;
even though the code is small and it is easy to come back and see that tmp is a
buffer, I prefer if you call it tmpBuffer, it is a good practice to do so
specially if the code can grow.

http://codereview.appspot.com/109068/diff/2034/2040
File src/common/buffer.h (right):

http://codereview.appspot.com/109068/diff/2034/2040#newcode486
src/common/buffer.h:486: Buffer CreateLimitedCopy (void) const;
can you please briefly say for what this function is used? what is a limited
copy, what does it remove from the full copy?

http://codereview.appspot.com/109068/diff/2034/2042
File src/common/packet-metadata.h (right):

http://codereview.appspot.com/109068/diff/2034/2042#newcode284
src/common/packet-metadata.h:284: 
please avoid white space changes

http://codereview.appspot.com/109068/diff/2034/2042#newcode319
src/common/packet-metadata.h:319: if (size > 0)
please add an initfunction instead of copy and paste initialization variables in
the different constructors.

http://codereview.appspot.com/109068/diff/2034/2043
File src/common/packet.cc (right):

http://codereview.appspot.com/109068/diff/2034/2043#newcode187
src/common/packet.cc:187: Packet::Packet (uint32_t mpiRank, uint32_t uid,
uint32_t size)
same thing here, please factorize the copy and paste init data into an
initfunction

http://codereview.appspot.com/109068/diff/2034/2043#newcode623
src/common/packet.cc:623: //XXX
fix that or remove commented code, same for all commented code

http://codereview.appspot.com/109068/diff/2034/2045
File src/contrib/net-anim/point-to-point-grid-helper.cc (right):

http://codereview.appspot.com/109068/diff/2034/2045#newcode82
src/contrib/net-anim/point-to-point-grid-helper.cc:82: for (uint32_t y = 0; y <
nRows; ++y)
coding style...

http://codereview.appspot.com/109068/diff/2034/2049
File src/devices/point-to-point/point-to-point-remote-channel.cc (right):

http://codereview.appspot.com/109068/diff/2034/2049#newcode55
src/devices/point-to-point/point-to-point-remote-channel.cc:55: 
please add an explicit empty destructor, not necessary but a good practice

http://codereview.appspot.com/109068/diff/2034/2054
File src/node/node.cc (right):

http://codereview.appspot.com/109068/diff/2034/2054#newcode97
src/node/node.cc:97: void Node::SetSystemId(uint32_t sid)
coding style

http://codereview.appspot.com/109068/diff/2034/2058
File src/simulator/default-simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/2034/2058#newcode63
src/simulator/default-simulator-impl.h:63: protected:
just to check, do you need all those variables/methods to be protected?

http://codereview.appspot.com/109068/diff/2034/2060
File src/simulator/distributed-simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/2034/2060#newcode40
src/simulator/distributed-simulator-impl.h:40: LBTSMessage()
please add destructor for all classes

http://codereview.appspot.com/109068/diff/2034/2060#newcode48
src/simulator/distributed-simulator-impl.h:48: uint32_t txCount;
You must have an excellent reason to make these variables public, please make
them private.

http://codereview.appspot.com/109068/diff/2034/2061
File src/simulator/mpi-interface.cc (right):

http://codereview.appspot.com/109068/diff/2034/2061#newcode140
src/simulator/mpi-interface.cc:140: #endif
why this and the following don't fail otherwise?
#else
NS_FATAL_ERROR ("Can't use distributed simulator without MPI compiled in");
#endif

http://codereview.appspot.com/109068/diff/2034/2062
File src/simulator/mpi-interface.h (right):

http://codereview.appspot.com/109068/diff/2034/2062#newcode44
src/simulator/mpi-interface.h:44: Buffer      buffer;
please use standard naming: m_buffer
Why they are public, should be private

http://codereview.appspot.com/109068/diff/2034/2062#newcode65
src/simulator/mpi-interface.h:65: public:
same as above

[Josh Pelkey, 2009-11-09 18:50:21]

Here are the updates after Faker's comments.

http://codereview.appspot.com/109068/diff/2034/2035
File examples/mpi/Makefile (right):

http://codereview.appspot.com/109068/diff/2034/2035#newcode4
examples/mpi/Makefile:4: # Change the NS3BASE below to the path of your base
ns-3
On 2009/11/06 14:22:30, faker.moatamri wrote:
> is your code compatible with the waf system?

I am discussing this issue on ns-developers.

http://codereview.appspot.com/109068/diff/2034/2036
File examples/mpi/README (right):

http://codereview.appspot.com/109068/diff/2034/2036#newcode23
examples/mpi/README:23: 4) Build ns-3 with the --with-mpi option
On 2009/11/06 14:22:30, faker.moatamri wrote:
> it would be nice that this command also builds the examples without having to
go
> through the following steps.

agreed. again, discussing on ns-developers.

http://codereview.appspot.com/109068/diff/2034/2037
File examples/mpi/nms-udp-nix.cc (right):

http://codereview.appspot.com/109068/diff/2034/2037#newcode1
examples/mpi/nms-udp-nix.cc:1: // DARPA NMS Campus Network Model
On 2009/11/06 14:22:30, faker.moatamri wrote:
> Files should begin with the licence

Done.

http://codereview.appspot.com/109068/diff/2034/2037#newcode5
examples/mpi/nms-udp-nix.cc:5: // - Link Bandwidths and Delays may not be the
same as the original
On 2009/11/06 14:22:30, faker.moatamri wrote:
> more details about the model implemented would be cool, for example a network
> topology scheme.

Done.

http://codereview.appspot.com/109068/diff/2034/2037#newcode51
examples/mpi/nms-udp-nix.cc:51: //RandomVariable::UseGlobalSeed (1, 1, 2, 3, 5,
8);
On 2009/11/06 14:22:30, faker.moatamri wrote:
> remove

Done.

http://codereview.appspot.com/109068/diff/2034/2037#newcode56
examples/mpi/nms-udp-nix.cc:56: uint32_t rank = MPIInterface::Rank();;
On 2009/11/06 14:22:30, faker.moatamri wrote:
> I know it's harmless but please remove the second semi-column

Done.

http://codereview.appspot.com/109068/diff/2034/2037#newcode83
examples/mpi/nms-udp-nix.cc:83: 
On 2009/11/06 14:22:30, faker.moatamri wrote:
> I think a topology of the network is more than necessary here

The new png hyperlinked above shows the topology.

http://codereview.appspot.com/109068/diff/2034/2037#newcode123
examples/mpi/nms-udp-nix.cc:123: nodes_net0[z][0].Add(nodes_net0[z][1].Get(0));
On 2009/11/06 14:22:30, faker.moatamri wrote:
> Coding style, mainly space before parentheses, also quotes, for example it
> should be 
> for ()
>   {
>     //code
>   }
> for all your files please, you can take a look at coding style page on
http://nsnam.org

Sorry, I fixed these at some point, but I guess I used an old file
version...uhg...fixed again.

http://codereview.appspot.com/109068/diff/2034/2039
File src/common/buffer.cc (right):

http://codereview.appspot.com/109068/diff/2034/2039#newcode661
src/common/buffer.cc:661: Buffer tmp;
On 2009/11/06 14:22:30, faker.moatamri wrote:
> even though the code is small and it is easy to come back and see that tmp is
a
> buffer, I prefer if you call it tmpBuffer, it is a good practice to do so
> specially if the code can grow.

Done.

http://codereview.appspot.com/109068/diff/2034/2040
File src/common/buffer.h (right):

http://codereview.appspot.com/109068/diff/2034/2040#newcode486
src/common/buffer.h:486: Buffer CreateLimitedCopy (void) const;
On 2009/11/06 14:22:30, faker.moatamri wrote:
> can you please briefly say for what this function is used? what is a limited
> copy, what does it remove from the full copy?

Done.

http://codereview.appspot.com/109068/diff/2034/2042
File src/common/packet-metadata.h (right):

http://codereview.appspot.com/109068/diff/2034/2042#newcode284
src/common/packet-metadata.h:284: 
On 2009/11/06 14:22:30, faker.moatamri wrote:
> please avoid white space changes

Done.

http://codereview.appspot.com/109068/diff/2034/2042#newcode319
src/common/packet-metadata.h:319: if (size > 0)
On 2009/11/06 14:22:30, faker.moatamri wrote:
> please add an initfunction instead of copy and paste initialization variables
in
> the different constructors.

Not sure what you mean here.  The only thing I added here was m_mpiRank and
m_packetUid initialization.  I did add the constructor below, and I was
following the examples of the other constructors.

http://codereview.appspot.com/109068/diff/2034/2043
File src/common/packet.cc (right):

http://codereview.appspot.com/109068/diff/2034/2043#newcode187
src/common/packet.cc:187: Packet::Packet (uint32_t mpiRank, uint32_t uid,
uint32_t size)
On 2009/11/06 14:22:30, faker.moatamri wrote:
> same thing here, please factorize the copy and paste init data into an
> initfunction

Again, not sure what you mean.  I was following the format of the other
constructors.  Note that this constructor is for re-creating packets that have
crossed simulator boundaries, which is why m_globalUid is not incremented.

http://codereview.appspot.com/109068/diff/2034/2043#newcode623
src/common/packet.cc:623: //XXX
On 2009/11/06 14:22:30, faker.moatamri wrote:
> fix that or remove commented code, same for all commented code

I was following the format of Serialize and Deserialize functions which already
exist (immediately below these two).  It looks like this hasn't been implemented
yet (it is also commented out), so I did the same.

http://codereview.appspot.com/109068/diff/2034/2045
File src/contrib/net-anim/point-to-point-grid-helper.cc (right):

http://codereview.appspot.com/109068/diff/2034/2045#newcode82
src/contrib/net-anim/point-to-point-grid-helper.cc:82: for (uint32_t y = 0; y <
nRows; ++y)
On 2009/11/06 14:22:30, faker.moatamri wrote:
> coding style...

Done.

http://codereview.appspot.com/109068/diff/2034/2049
File src/devices/point-to-point/point-to-point-remote-channel.cc (right):

http://codereview.appspot.com/109068/diff/2034/2049#newcode55
src/devices/point-to-point/point-to-point-remote-channel.cc:55: 
On 2009/11/06 14:22:30, faker.moatamri wrote:
> please add an explicit empty destructor, not necessary but a good practice

Done.

http://codereview.appspot.com/109068/diff/2034/2054
File src/node/node.cc (right):

http://codereview.appspot.com/109068/diff/2034/2054#newcode97
src/node/node.cc:97: void Node::SetSystemId(uint32_t sid)
On 2009/11/06 14:22:30, faker.moatamri wrote:
> coding style

Done.

http://codereview.appspot.com/109068/diff/2034/2058
File src/simulator/default-simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/2034/2058#newcode63
src/simulator/default-simulator-impl.h:63: protected:
On 2009/11/06 14:22:30, faker.moatamri wrote:
> just to check, do you need all those variables/methods to be protected?

Done.

http://codereview.appspot.com/109068/diff/2034/2060
File src/simulator/distributed-simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/2034/2060#newcode40
src/simulator/distributed-simulator-impl.h:40: LBTSMessage()
On 2009/11/06 14:22:30, faker.moatamri wrote:
> please add destructor for all classes

Done.

http://codereview.appspot.com/109068/diff/2034/2060#newcode48
src/simulator/distributed-simulator-impl.h:48: uint32_t txCount;
On 2009/11/06 14:22:30, faker.moatamri wrote:
> You must have an excellent reason to make these variables public, please make
> them private.

Done.

http://codereview.appspot.com/109068/diff/2034/2061
File src/simulator/mpi-interface.cc (right):

http://codereview.appspot.com/109068/diff/2034/2061#newcode140
src/simulator/mpi-interface.cc:140: #endif
On 2009/11/06 14:22:30, faker.moatamri wrote:
> why this and the following don't fail otherwise?
> #else
> NS_FATAL_ERROR ("Can't use distributed simulator without MPI compiled in");
> #endif

Done.

http://codereview.appspot.com/109068/diff/2034/2062
File src/simulator/mpi-interface.h (right):

http://codereview.appspot.com/109068/diff/2034/2062#newcode44
src/simulator/mpi-interface.h:44: Buffer      buffer;
On 2009/11/06 14:22:30, faker.moatamri wrote:
> please use standard naming: m_buffer
> Why they are public, should be private

Done.

http://codereview.appspot.com/109068/diff/2034/2062#newcode65
src/simulator/mpi-interface.h:65: public:
On 2009/11/06 14:22:30, faker.moatamri wrote:
> same as above

Done.

[faker.moatamri, 2009-11-09 21:17:26]

I hope that I answered all your questions. If you need more clarifications
please don't hesitate.

http://codereview.appspot.com/109068/diff/2034/2043
File src/common/packet.cc (right):

http://codereview.appspot.com/109068/diff/2034/2043#newcode187
src/common/packet.cc:187: Packet::Packet (uint32_t mpiRank, uint32_t uid,
uint32_t size)
Yes I know probably not you fault but actually those constructors have a lot of
common code that can be factorized into an init function which will be called by
all the constructors, it's not that the actual code is bad, it's just that there
is a lot of copy and paste between the different versions of the constructor.

http://codereview.appspot.com/109068/diff/2034/2043#newcode623
src/common/packet.cc:623: //XXX
commented code should not be included in code base. If it is commented, either
we uncomment it or we delete. What happens with commented code is that usually,
it is left there for ages and no one remembers why it is there, and it ends up
as a garbage unuseful code.

[craigdo, 2009-11-13 20:20:02]

Mostly small stuff, but the bit about putting mpi-specific stuff in generic
classes bothers me just like it did in the nix case.

I'm going to play around with running the code some more.

http://codereview.appspot.com/109068/diff/6010/5041
File examples/mpi/Makefile (right):

http://codereview.appspot.com/109068/diff/6010/5041#newcode1
examples/mpi/Makefile:1: #
#################################################################
I'm sure it's been mentioned, but this should be done in waf

http://codereview.appspot.com/109068/diff/6010/5042
File examples/mpi/README (right):

http://codereview.appspot.com/109068/diff/6010/5042#newcode9
examples/mpi/README:9: 1) Ensure that MPI is installed, as well as mpic++.  In
Ubuntu
Maybe mention Fedora conflict and workaround here.

http://codereview.appspot.com/109068/diff/6010/5042#newcode13
examples/mpi/README:13: 2) Set the CXX environment variable to the path of
mpic++.
This should be a waf thing (and remove the makefile)

http://codereview.appspot.com/109068/diff/6010/5042#newcode17
examples/mpi/README:17: 3) Configure ns-3 with the --with-mpi option
--with-xxx is typically used to provide a path, as in --with-nsc=../nsc

I think this should be --enable-mpi for consistency

http://codereview.appspot.com/109068/diff/6010/5042#newcode23
examples/mpi/README:23: 4) Build ns-3 with the --with-mpi option
Why do you have to build with an option.  In every other case, the configuration
is sticky.

http://codereview.appspot.com/109068/diff/6010/5042#newcode40
examples/mpi/README:40: - LD_LIBRARY_PATH should be set to the debug
If you use ./waf --run or ./waf shell this is taken care of for you.

http://codereview.appspot.com/109068/diff/6010/5042#newcode42
examples/mpi/README:42: Ex: export
'LD_LIBRARY_PATH=/home/user/repos/ns-3-distributed/build/debug:/usr/lib/openmpi/1.2.7-gcc/lib'
We have avoided setting the LD_LIBRARY_PATH this way.  What happens if you have
multiple ns-3 builds?

Should really use ./waf --run or ./waf shell instead

http://codereview.appspot.com/109068/diff/6010/5042#newcode53
examples/mpi/README:53: 3) Edit the Makefile in the examples/mpi directory.  The
Noooooo.  Please, no Makefile that I have to edit!

http://codereview.appspot.com/109068/diff/6010/5042#newcode57
examples/mpi/README:57: 4) Build the programs with make.
Nooooo.  Please, no Makefile.

http://codereview.appspot.com/109068/diff/6010/5042#newcode59
examples/mpi/README:59: 5) The programs are now ready to run with mpirun.  Here
are
I think it would be better to keep this in the ns-3 waf world.  How about 

  ./waf --run "mpirun -np2 ./test-dirstributed ..."

http://codereview.appspot.com/109068/diff/6010/5042#newcode63
examples/mpi/README:63: mpirun -np 4 -machinefile mpihosts ./nms-udp-nix --LAN=2
--CN=2 --NIX=1
If you provide these parameters, the simulation errors out complaining about
number of CNs.

You also might want to warn people that this might take an hour or two to run
depending on their machine.

http://codereview.appspot.com/109068/diff/6010/5043
File examples/mpi/nms-udp-nix.cc (right):

http://codereview.appspot.com/109068/diff/6010/5043#newcode25
examples/mpi/nms-udp-nix.cc:25: * (c) 2009, GTech Systems, Inc. - Alfred Park
<park@gtech-systems.com>
Shouldn't copyright go up top?

http://codereview.appspot.com/109068/diff/6010/5044
File examples/mpi/test-distributed.cc (right):

http://codereview.appspot.com/109068/diff/6010/5044#newcode72
examples/mpi/test-distributed.cc:72: // Must have 2 and only 2 LPs
How about "2 and only 2 Logical Processors (LPs)

Do you expect people to know MPI jargon?

http://codereview.appspot.com/109068/diff/6010/5044#newcode95
examples/mpi/test-distributed.cc:95: Ptr<Node> node = CreateObject<Node> (0);
Shouldn't system ID be an attribute?  Node containers could use an object
factory and take attributes in a setter (point-to-point-helper, many others
already do this) as in,

  NodeContainer nodes;
  nodes.SetAttribute ("SystemId", UintegerValue (1));
  nodes.Create (4);

http://codereview.appspot.com/109068/diff/6010/5044#newcode103
examples/mpi/test-distributed.cc:103: Ptr<Node> routerNode1 = CreateObject<Node>
(0);
This is handy.  Could also use more verbose CreateObjectWithAttributes<Node>
("SystemId", UintegerValue (0)) for clarity if systemId were an attribute.

http://codereview.appspot.com/109068/diff/6010/5045
File src/common/buffer.cc (right):

http://codereview.appspot.com/109068/diff/6010/5045#newcode656
src/common/buffer.cc:656: Buffer::CreateLimitedCopy (void) const
I don't think this method name is very clear.  I tend to think truncation when I
see this.  

Buffer::CreateCompressedCopy?
Buffer::CreateCopyWithoutPadding?
Buffer::CopyWithoutZeroPadding?
Buffer::CopyEncodeZeroPadding?
Buffer::CopyCompress?

http://codereview.appspot.com/109068/diff/6010/5046
File src/common/buffer.h (right):

http://codereview.appspot.com/109068/diff/6010/5046#newcode499
src/common/buffer.h:499: Buffer CreateLimitedCopy (void) const;
There is nothing really limited about this copy.  The padding is just encoded
differently, right?

http://codereview.appspot.com/109068/diff/6010/5047
File src/common/packet-metadata.cc (right):

http://codereview.appspot.com/109068/diff/6010/5047#newcode968
src/common/packet-metadata.cc:968: PacketMetadata::GetMpiRank (void) const
Explicit reference to a particular flavor of packet in a generic PacketMetadata
class.

I don't think an MPI rank belongs here any more than I think a nix vector
belongs in Packet.

http://codereview.appspot.com/109068/diff/6010/5048
File src/common/packet-metadata.h (right):

http://codereview.appspot.com/109068/diff/6010/5048#newcode131
src/common/packet-metadata.h:131: inline PacketMetadata (uint32_t mpiRank,
uint32_t uid, uint32_t size);
I don't think it's wise to start stuffing things like this in our generic
classes.

http://codereview.appspot.com/109068/diff/6010/5049
File src/common/packet.cc (right):

http://codereview.appspot.com/109068/diff/6010/5049#newcode187
src/common/packet.cc:187: Packet::Packet (uint32_t mpiRank, uint32_t uid,
uint32_t size)
Okay, this means we have nix-specific code in our packets; and now mpi-specific
code.  Where does this end?

We have a P1 bug to remove the nix stuff, right?  This will turn into a P1 bug
as soon as it's checked in.

http://codereview.appspot.com/109068/diff/6010/5049#newcode578
src/common/packet.cc:578: Packet::SerializeForMpi (void) const
There's got to be a better way.  This is the beginning of a fragile base class
problem, IMO.

If we need to provide a new mechanism, we should do this now.

http://codereview.appspot.com/109068/diff/6010/5050
File src/common/packet.h (right):

http://codereview.appspot.com/109068/diff/6010/5050#newcode244
src/common/packet.h:244: Packet (uint32_t mpiRank, uint32_t uid, uint32_t size);
I have problems with putting this into a generic packet class.

http://codereview.appspot.com/109068/diff/6010/5050#newcode441
src/common/packet.h:441: Buffer SerializeForMpi (void) const;
I think this should move to an MpiPacket or there should be some way to
associate this information with Packet.

Creating MpiPacket instead of Packet using ObjectFactory approach seems clean.

We agreed that the nix stuff would be pulled out for ns-3.7 so why are we
immediately putting more stuff in?

http://codereview.appspot.com/109068/diff/6010/5051
File src/contrib/net-anim/point-to-point-grid-helper.cc (right):

http://codereview.appspot.com/109068/diff/6010/5051#newcode76
src/contrib/net-anim/point-to-point-grid-helper.cc:76:
PointToPointGridHelper::PointToPointGridHelper (uint32_t nRows,
Could make this a compile-time error by moving the ifdef around the entire
method?

http://codereview.appspot.com/109068/diff/6010/5055
File src/devices/point-to-point/point-to-point-remote-channel.cc (right):

http://codereview.appspot.com/109068/diff/6010/5055#newcode77
src/devices/point-to-point/point-to-point-remote-channel.cc:77: #ifdef NS3_MPI
Shouldn't this fatal error if it won't work rather than just not do something
silently?

http://codereview.appspot.com/109068/diff/6010/5063
File src/simulator/default-simulator-impl.cc (right):

http://codereview.appspot.com/109068/diff/6010/5063#newcode103
src/simulator/default-simulator-impl.cc:103: uint32_t
DefaultSimulatorImpl::GetSystemId () const
Why not in distributed version?

http://codereview.appspot.com/109068/diff/6010/5065
File src/simulator/distributed-simulator-impl.cc (right):

http://codereview.appspot.com/109068/diff/6010/5065#newcode41
src/simulator/distributed-simulator-impl.cc:41: using namespace std;
We typically don't do this.

http://codereview.appspot.com/109068/diff/6010/5065#newcode88
src/simulator/distributed-simulator-impl.cc:88: #ifdef NS3_MPI
Every time I look at this construct, I think to myself, "there's got to be a
better way."

http://codereview.appspot.com/109068/diff/6010/5067
File src/simulator/mpi-interface.cc (right):

http://codereview.appspot.com/109068/diff/6010/5067#newcode38
src/simulator/mpi-interface.cc:38: using namespace std;
We typically don't do this.

http://codereview.appspot.com/109068/diff/6010/5072
File wscript (right):

http://codereview.appspot.com/109068/diff/6010/5072#newcode197
wscript:197: opt.add_option('--with-mpi',
I think this should be --enable-mpi for consistency

[Tom Henderson, 2009-11-16 05:17:31]

summary of main questions/comments:
- it seemed to me that Packet/PacketMetadata might not need to be touched-- can
you please have a look at that issue in the review below?
- src/simulator module would acquire dependencies on src/common and src/devices
if mpi-interface.cc is left in src/simulator.  Should mpi-interface.cc be in
src/devices or a new src/mpi directory?
- what happens if users do not interconnect nodes with MPI-capable interfaces,
due to misconfiguration or lack of understanding of the MPI applicability-- how
do programs behave (for instance, a Wifi channel with nodes on different
systems)?
- what should be the behavior when global routing and MPI are both enabled by
user?  Perhaps this should be documented more than the one-line "continue" in
the code.
- needs manual chapter or better documentation, before release (not necessary to
merge, though)

I am not repeating the other comments left by Craig about build system, etc.
with which I agree.

http://codereview.appspot.com/109068/diff/6010/5043
File examples/mpi/nms-udp-nix.cc (right):

http://codereview.appspot.com/109068/diff/6010/5043#newcode53
examples/mpi/nms-udp-nix.cc:53: #endif
I wonder whether these guards are really necessary in the example programs.  Is
the goal to make them build even if there is no MPI?

http://codereview.appspot.com/109068/diff/6010/5043#newcode418
examples/mpi/nms-udp-nix.cc:418: else if (rank == z%systemCount)
Is there a reason that the example instantiates all objects on all systems, and
then just turns some of the applications on selectively? Would a better example
be a program that instead only instantiated the necessary objects on each
system?

http://codereview.appspot.com/109068/diff/6010/5043#newcode541
examples/mpi/nms-udp-nix.cc:541: #if 0
Suggest to put this block of code into a conditional 
if (tracing)
  {
    ...
  }

and enable via command line.

see examples/wireless/wifi-simple-adhoc-grid.cc, for instance.

http://codereview.appspot.com/109068/diff/6010/5044
File examples/mpi/test-distributed.cc (right):

http://codereview.appspot.com/109068/diff/6010/5044#newcode215
examples/mpi/test-distributed.cc:215: Address sinkLocalAddress(InetSocketAddress
(Ipv4Address::GetAny (), port));
by the way, I found some stray missing spaces throughout the whole patchset.  I
wonder whether the GNU indent with -prs option can be used to catch these.

http://codereview.appspot.com/109068/diff/6010/5048
File src/common/packet-metadata.h (right):

http://codereview.appspot.com/109068/diff/6010/5048#newcode156
src/common/packet-metadata.h:156: uint32_t GetMpiRank (void) const;
This method is not used by any code.  I wonder whether changes to PacketMetadata
or Packet are really needed for MPI.  Does Packet really need to be associated
with a system ID at all times?  It seems to me to try for some refactoring that
avoids touching class Packet or PacketMetadata.  More on this below...

http://codereview.appspot.com/109068/diff/6010/5055
File src/devices/point-to-point/point-to-point-remote-channel.cc (right):

http://codereview.appspot.com/109068/diff/6010/5055#newcode78
src/devices/point-to-point/point-to-point-remote-channel.cc:78:
MPIInterface::SendPacket(p, rxTime, dst->GetNode()->GetId(), dst->GetIfIndex());
Here, you can call src->GetNode ()->GetSystemId () to get the system ID and then
pass as argument to SendPacket().  Would that avoid packet needing to carry
system ID?

http://codereview.appspot.com/109068/diff/6010/5062
File src/routing/global-routing/global-route-manager-impl.cc (right):

http://codereview.appspot.com/109068/diff/6010/5062#newcode552
src/routing/global-routing/global-route-manager-impl.cc:552: if
(node->GetSystemId() != MPIInterface::Rank()) continue;
In the example that instantiated every object everywhere and then only turned on
applications on the relevant system ID, wouldn't global routing still work in
that case?

Need to think about how global routing should interact with MPI.  Should global
routing be extended to handle MPI?  Should it be disabled by default when MPI is
enabled, forcing user to think about whether it will work for the coded
scenario?  The above code will cause the global routing to partially complete
but not return an error.

http://codereview.appspot.com/109068/diff/6010/5066
File src/simulator/distributed-simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/6010/5066#newcode61
src/simulator/distributed-simulator-impl.h:61: 
doxygen missing

http://codereview.appspot.com/109068/diff/6010/5067
File src/simulator/mpi-interface.cc (right):

http://codereview.appspot.com/109068/diff/6010/5067#newcode28
src/simulator/mpi-interface.cc:28: #include "ns3/point-to-point-net-device.h"
This class shouldn't be in src/simulator if it depends on
point-to-point-net-device.h.  Even making this class depend on class
Packet/Buffer introduces a new dependency on src/simulator (depends on
src/common) that wasn't there before.

[Mathieu Lacage, 2009-11-17 14:53:19]

I have two main general comments:

1) It's really not cool that my previous comments have not been addressed before
submitting this patch for review.

2) I do not understand the need for the per-packet system id: I am not really
against it for now but I want to understand what bug it is fixing before ack-ing
it. i.e., what happens if you remove this part of the submitted patch ?

http://codereview.appspot.com/109068/diff/6010/5046
File src/common/buffer.h (right):

http://codereview.appspot.com/109068/diff/6010/5046#newcode499
src/common/buffer.h:499: Buffer CreateLimitedCopy (void) const;
rename to Serialize, add Deserialize and GetSerializedSize and use the same
signatures as the signatures I suggested for Packet.

I am almost certain I already suggested this previously too.

http://codereview.appspot.com/109068/diff/6010/5050
File src/common/packet.h (right):

http://codereview.appspot.com/109068/diff/6010/5050#newcode441
src/common/packet.h:441: Buffer SerializeForMpi (void) const;
I already suggested at least twice by email the alternate correct API for this.
Here is the relevant extract of my email:

--------------------------------------------

uint32_t Packet::GetSerializedSize (void) const;
// returns zero if input buffer was too small.
uint32_t Packet::Serialize (uint8_t *buffer, uint32_t size) const;
// returns zero if input buffer did not contain a complete message.
uint32_t Packet::Deserialize (uint8_t *buffer, uint32_t size);

You need to get hold of a tx buffer big enough but, at least, you know when and
where it is allocated and you can implement a per-mpi-request allocation policy
if you wish
to.
--------------------------------------------

I fail to see the point of code reviews if the resulting suggestions are
ignored/not acted upon. This takes a lot of time for everyone so, whenever you
re-submit a version of your code which does not address the
previously-identified issues, you make it less likely we will re-review the same
code with the same problems.

http://codereview.appspot.com/109068/diff/6010/5054
File src/devices/point-to-point/point-to-point-channel.h (right):

http://codereview.appspot.com/109068/diff/6010/5054#newcode100
src/devices/point-to-point/point-to-point-channel.h:100: protected:
That protected keyword leaves a really bad taste in my mouth. You could
trivially add a 'dst' argument to TransmitStart and leave the private in.

http://codereview.appspot.com/109068/diff/6010/5064
File src/simulator/default-simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/6010/5064#newcode63
src/simulator/default-simulator-impl.h:63: protected:
Please, don't try to reuse this class for your simulator implementation.
Instead, you can copy/paste default-simulator-impl.* into
distributed-simulator-impl* and modify it to fit your needs. Yes, this brings
code duplication but it makes it easier to manage separately these separate
functionalities.

http://codereview.appspot.com/109068/diff/6010/5067
File src/simulator/mpi-interface.cc (right):

http://codereview.appspot.com/109068/diff/6010/5067#newcode26
src/simulator/mpi-interface.cc:26: #include "ns3/node.h"
the simulator module cannot depend on the node module. What we have done to deal
with this in the multithreading work is to move this node-specific code in a
separate module.

http://codereview.appspot.com/109068/diff/6010/5068
File src/simulator/mpi-interface.h (right):

http://codereview.appspot.com/109068/diff/6010/5068#newcode39
src/simulator/mpi-interface.h:39: class SentBuffer {
this class appears to be an implementation detail of this API so, it probably
would be best to defer its definition to the .cc file

http://codereview.appspot.com/109068/diff/6010/5068#newcode58
src/simulator/mpi-interface.h:58: class MPIInterface
To avoid exporting the mpi.h header publicly, I would suggest to use the
singleton and pimpl idioms. Namely:

in the .h:

class DistributedManager
{
public:
  static DistributedManager *Get (void);

  uint32_t GetRank (void);
  ...
private:
  DistributedManagerImpl *m_impl;
};

in the .cc:

class DistributedManagerImpl {
public:
  ...
};

DistributedManager *
DistributedManager::Get (void)
{
  static DistributedManager manager;
  return &manager;
}

And yes, I think that you need to pick a better name than MpiInterface or
DistributedManager.

http://codereview.appspot.com/109068/diff/6010/5070
File src/simulator/simulator.h (right):

http://codereview.appspot.com/109068/diff/6010/5070#newcode618
src/simulator/simulator.h:618: static Ptr<SimulatorImpl>* PeekImpl(void);
what is wrong with SetImplementation and GetImplementation ? These additions
appear to be redundant with them.

[Josh Pelkey, 2009-12-10 18:42:47]

Hi all,

Sorry for the delay after the comments.  We were working on switching the
serialization API, according to Mathieu's comments.  Since it has been a while,
I will provide a brief summary of the changes and the remaining issues.

*******
Changes
*******

-- Various small fixes according to the comments (code style, typos, etc, which
I responded to in the comments at the bottom of this reply)

-- Buffer API.  Switched the serialization/deserialization methods to use a raw
buffer.  Added methods to buffer, nix-vector, packet-metadata, and packet to
support this

-- Changed distributed-simulator-impl to use simulator-impl rather than
default-simulator-impl (per Mathieu's comment). In order to do this, we
introduced a pure virtual method in simulator-impl for GetSystemId.  This method
was added to default-simulator-impl, distributed-simulator-impl, and
realtime-simulator-impl

*******
Issues
*******

-- Building with WAF.  I want to do this, and I've tried several times to get
something to work.  I know it is possible, but I'm just not that good at using
WAF.  I need to re-start the discussion on ns-developers for help with this.

-- Dependency issues.  Tom and Mathieu commented on dependencies in
src/simulator/distributed-simulator-impl and/or mpi-interface.  We are open to
moving these classes wherever they fit best.

-- Rank in packet-metadata.  We are still not sure why UID in packet-metadata
fits, but rank does not.  Tom has pointed out that we could get away with taking
it out for now, since it isn't used.  However, in the future, it will be
necessary.  So I feel like we should figure out where it needs to go, especially
now that we aren't constrained by a tight release schedule (since we missed
ns-3.7).

-- new assert in node.cc causing distributed examples to seg. fault
(NS_ASSERT_MSG (Simulator::GetContext () == GetId (),...).  I don't even know
what this does, so I'm not sure if it is a problem with our code, or if it is a
problem with the assert.  All I know is if I take it out, the examples run fine.

-- stability issues.  Craig sent an email about his concerns regarding stability
in MPI.  I think there was a problem with an example script that he created
because he was trying to run MPI with non-point-to-point links.  Currently, this
MPI code only works for point-to-point links.  We will have to stress this in
the manual or tutorial when we get to that point.

http://codereview.appspot.com/109068/diff/6010/5041
File examples/mpi/Makefile (right):

http://codereview.appspot.com/109068/diff/6010/5041#newcode1
examples/mpi/Makefile:1: #
#################################################################
On 2009/11/13 20:20:02, craigdo wrote:
> I'm sure it's been mentioned, but this should be done in waf

Agreed.

http://codereview.appspot.com/109068/diff/6010/5042
File examples/mpi/README (right):

http://codereview.appspot.com/109068/diff/6010/5042#newcode9
examples/mpi/README:9: 1) Ensure that MPI is installed, as well as mpic++.  In
Ubuntu
On 2009/11/13 20:20:02, craigdo wrote:
> Maybe mention Fedora conflict and workaround here.

Done.

http://codereview.appspot.com/109068/diff/6010/5042#newcode13
examples/mpi/README:13: 2) Set the CXX environment variable to the path of
mpic++.
On 2009/11/13 20:20:02, craigdo wrote:
> This should be a waf thing (and remove the makefile)

Agreed.

http://codereview.appspot.com/109068/diff/6010/5042#newcode17
examples/mpi/README:17: 3) Configure ns-3 with the --with-mpi option
On 2009/11/13 20:20:02, craigdo wrote:
> --with-xxx is typically used to provide a path, as in --with-nsc=../nsc
> 
> I think this should be --enable-mpi for consistency

Done.

http://codereview.appspot.com/109068/diff/6010/5042#newcode23
examples/mpi/README:23: 4) Build ns-3 with the --with-mpi option
On 2009/11/13 20:20:02, craigdo wrote:
> Why do you have to build with an option.  In every other case, the
configuration
> is sticky.

You are right...I forgot about this.

http://codereview.appspot.com/109068/diff/6010/5042#newcode40
examples/mpi/README:40: - LD_LIBRARY_PATH should be set to the debug
On 2009/11/13 20:20:02, craigdo wrote:
> If you use ./waf --run or ./waf shell this is taken care of for you.

Right again :)

http://codereview.appspot.com/109068/diff/6010/5042#newcode42
examples/mpi/README:42: Ex: export
'LD_LIBRARY_PATH=/home/user/repos/ns-3-distributed/build/debug:/usr/lib/openmpi/1.2.7-gcc/lib'
On 2009/11/13 20:20:02, craigdo wrote:
> We have avoided setting the LD_LIBRARY_PATH this way.  What happens if you
have
> multiple ns-3 builds?
> 
> Should really use ./waf --run or ./waf shell instead

Agreed.

http://codereview.appspot.com/109068/diff/6010/5042#newcode53
examples/mpi/README:53: 3) Edit the Makefile in the examples/mpi directory.  The
On 2009/11/13 20:20:02, craigdo wrote:
> Noooooo.  Please, no Makefile that I have to edit!

:)

http://codereview.appspot.com/109068/diff/6010/5042#newcode59
examples/mpi/README:59: 5) The programs are now ready to run with mpirun.  Here
are
On 2009/11/13 20:20:02, craigdo wrote:
> I think it would be better to keep this in the ns-3 waf world.  How about 
> 
>   ./waf --run "mpirun -np2 ./test-dirstributed ..."

Ok, I definitely agree with this and would like it to work this way.  The
problem is...I don't know how.

http://codereview.appspot.com/109068/diff/6010/5042#newcode63
examples/mpi/README:63: mpirun -np 4 -machinefile mpihosts ./nms-udp-nix --LAN=2
--CN=2 --NIX=1
On 2009/11/13 20:20:02, craigdo wrote:
> If you provide these parameters, the simulation errors out complaining about
> number of CNs.
> 
> You also might want to warn people that this might take an hour or two to run
> depending on their machine.

Whoops, number of logical processors needs to be >= number of campus networks. 
Fixed.

http://codereview.appspot.com/109068/diff/6010/5043
File examples/mpi/nms-udp-nix.cc (right):

http://codereview.appspot.com/109068/diff/6010/5043#newcode25
examples/mpi/nms-udp-nix.cc:25: * (c) 2009, GTech Systems, Inc. - Alfred Park
<park@gtech-systems.com>
On 2009/11/13 20:20:02, craigdo wrote:
> Shouldn't copyright go up top?

Done.

http://codereview.appspot.com/109068/diff/6010/5043#newcode53
examples/mpi/nms-udp-nix.cc:53: #endif
On 2009/11/16 05:17:31, Tom Henderson wrote:
> I wonder whether these guards are really necessary in the example programs. 
Is
> the goal to make them build even if there is no MPI?

I think the issue involves mpi.h possibly not existing.  By checking for
NS3_MPI, we know that the user has enabled MPI, and WAF has checked to make sure
that they have MPI installed.  The goal was not to build if there is no MPI but
to not fail if MPI is not enabled.

You are right though, these examples don't need to be built at all if MPI is not
enabled.  I imagine this can be handled in the wscript somehow.

http://codereview.appspot.com/109068/diff/6010/5043#newcode418
examples/mpi/nms-udp-nix.cc:418: else if (rank == z%systemCount)
On 2009/11/16 05:17:31, Tom Henderson wrote:
> Is there a reason that the example instantiates all objects on all systems,
and
> then just turns some of the applications on selectively? Would a better
example
> be a program that instead only instantiated the necessary objects on each
> system?

Yes, the most efficient design would only instantiate objects on the appropriate
system.  However, for ease-of-use with existing routing protocols, the full
topology is created on each system.  Only the applications are system-specific. 
This way a routing protocol like global routing can work with MPI.

http://codereview.appspot.com/109068/diff/6010/5043#newcode541
examples/mpi/nms-udp-nix.cc:541: #if 0
On 2009/11/16 05:17:31, Tom Henderson wrote:
> Suggest to put this block of code into a conditional 
> if (tracing)
>   {
>     ...
>   }
> 
> and enable via command line.
> 
> see examples/wireless/wifi-simple-adhoc-grid.cc, for instance.

Done.

http://codereview.appspot.com/109068/diff/6010/5044
File examples/mpi/test-distributed.cc (right):

http://codereview.appspot.com/109068/diff/6010/5044#newcode72
examples/mpi/test-distributed.cc:72: // Must have 2 and only 2 LPs
On 2009/11/13 20:20:02, craigdo wrote:
> How about "2 and only 2 Logical Processors (LPs)
> 
> Do you expect people to know MPI jargon?

Done.

http://codereview.appspot.com/109068/diff/6010/5044#newcode95
examples/mpi/test-distributed.cc:95: Ptr<Node> node = CreateObject<Node> (0);
On 2009/11/13 20:20:02, craigdo wrote:
> Shouldn't system ID be an attribute?  Node containers could use an object
> factory and take attributes in a setter (point-to-point-helper, many others
> already do this) as in,
> 
>   NodeContainer nodes;
>   nodes.SetAttribute ("SystemId", UintegerValue (1));
>   nodes.Create (4);
> 

I was using the API that already existed.  Should it be changed?

http://codereview.appspot.com/109068/diff/6010/5044#newcode103
examples/mpi/test-distributed.cc:103: Ptr<Node> routerNode1 = CreateObject<Node>
(0);
On 2009/11/13 20:20:02, craigdo wrote:
> This is handy.  Could also use more verbose CreateObjectWithAttributes<Node>
> ("SystemId", UintegerValue (0)) for clarity if systemId were an attribute.

I'm guessing you would like it to be :)

http://codereview.appspot.com/109068/diff/6010/5044#newcode215
examples/mpi/test-distributed.cc:215: Address sinkLocalAddress(InetSocketAddress
(Ipv4Address::GetAny (), port));
On 2009/11/16 05:17:31, Tom Henderson wrote:
> by the way, I found some stray missing spaces throughout the whole patchset. 
I
> wonder whether the GNU indent with -prs option can be used to catch these.

I tried using GNU indent once but failed.  I will look into it again.  It
doesn't look like "prs" is the option though.  That puts a space after open
parenthesis and before close ( like this ).  I'll try using "-gnu --no-tabs".

http://codereview.appspot.com/109068/diff/6010/5045
File src/common/buffer.cc (right):

http://codereview.appspot.com/109068/diff/6010/5045#newcode656
src/common/buffer.cc:656: Buffer::CreateLimitedCopy (void) const
On 2009/11/13 20:20:02, craigdo wrote:
> I don't think this method name is very clear.  I tend to think truncation when
I
> see this.  
> 
> Buffer::CreateCompressedCopy?
> Buffer::CreateCopyWithoutPadding?
> Buffer::CopyWithoutZeroPadding?
> Buffer::CopyEncodeZeroPadding?
> Buffer::CopyCompress?

We have removed this method completely, since we are using the API that Mathieu
suggested.

http://codereview.appspot.com/109068/diff/6010/5046
File src/common/buffer.h (right):

http://codereview.appspot.com/109068/diff/6010/5046#newcode499
src/common/buffer.h:499: Buffer CreateLimitedCopy (void) const;
On 2009/11/13 20:20:02, craigdo wrote:
> There is nothing really limited about this copy.  The padding is just encoded
> differently, right?

Again, this method has been removed.

http://codereview.appspot.com/109068/diff/6010/5051
File src/contrib/net-anim/point-to-point-grid-helper.cc (right):

http://codereview.appspot.com/109068/diff/6010/5051#newcode76
src/contrib/net-anim/point-to-point-grid-helper.cc:76:
PointToPointGridHelper::PointToPointGridHelper (uint32_t nRows,
On 2009/11/13 20:20:02, craigdo wrote:
> Could make this a compile-time error by moving the ifdef around the entire
> method?

I just deleted this method, because I haven't had the chance to test it enough.

http://codereview.appspot.com/109068/diff/6010/5054
File src/devices/point-to-point/point-to-point-channel.h (right):

http://codereview.appspot.com/109068/diff/6010/5054#newcode100
src/devices/point-to-point/point-to-point-channel.h:100: protected:
On 2009/11/17 14:53:19, Mathieu Lacage wrote:
> That protected keyword leaves a really bad taste in my mouth. You could
> trivially add a 'dst' argument to TransmitStart and leave the private in.

Done.

http://codereview.appspot.com/109068/diff/6010/5055
File src/devices/point-to-point/point-to-point-remote-channel.cc (right):

http://codereview.appspot.com/109068/diff/6010/5055#newcode77
src/devices/point-to-point/point-to-point-remote-channel.cc:77: #ifdef NS3_MPI
On 2009/11/13 20:20:02, craigdo wrote:
> Shouldn't this fatal error if it won't work rather than just not do something
> silently?

Done.

http://codereview.appspot.com/109068/diff/6010/5062
File src/routing/global-routing/global-route-manager-impl.cc (right):

http://codereview.appspot.com/109068/diff/6010/5062#newcode552
src/routing/global-routing/global-route-manager-impl.cc:552: if
(node->GetSystemId() != MPIInterface::Rank()) continue;
On 2009/11/16 05:17:31, Tom Henderson wrote:
> In the example that instantiated every object everywhere and then only turned
on
> applications on the relevant system ID, wouldn't global routing still work in
> that case?
> 
> Need to think about how global routing should interact with MPI.  Should
global
> routing be extended to handle MPI?  Should it be disabled by default when MPI
is
> enabled, forcing user to think about whether it will work for the coded
> scenario?  The above code will cause the global routing to partially complete
> but not return an error.

Yes, global routing will work with MPI.  Since the full topology is created on
every logical processor, global routing computes routes for any node assigned to
the logical processor.  It does not compute routes from nodes that are not
associated with this logical processor, since packets will never originate from
these nodes on this LP.

http://codereview.appspot.com/109068/diff/6010/5063
File src/simulator/default-simulator-impl.cc (right):

http://codereview.appspot.com/109068/diff/6010/5063#newcode103
src/simulator/default-simulator-impl.cc:103: uint32_t
DefaultSimulatorImpl::GetSystemId () const
On 2009/11/13 20:20:02, craigdo wrote:
> Why not in distributed version?

This method has been changed to pure virtual in the simulator-impl base class. 
DefaultSimulator, DistributedSimulator, and RealTimeSimultor have been modified
to use this method.  It should return zero by default (non-distributed case).

http://codereview.appspot.com/109068/diff/6010/5064
File src/simulator/default-simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/6010/5064#newcode63
src/simulator/default-simulator-impl.h:63: protected:
On 2009/11/17 14:53:19, Mathieu Lacage wrote:
> Please, don't try to reuse this class for your simulator implementation.
> Instead, you can copy/paste default-simulator-impl.* into
> distributed-simulator-impl* and modify it to fit your needs. Yes, this brings
> code duplication but it makes it easier to manage separately these separate
> functionalities.

Done.

http://codereview.appspot.com/109068/diff/6010/5065
File src/simulator/distributed-simulator-impl.cc (right):

http://codereview.appspot.com/109068/diff/6010/5065#newcode41
src/simulator/distributed-simulator-impl.cc:41: using namespace std;
On 2009/11/13 20:20:02, craigdo wrote:
> We typically don't do this.

Done.

http://codereview.appspot.com/109068/diff/6010/5067
File src/simulator/mpi-interface.cc (right):

http://codereview.appspot.com/109068/diff/6010/5067#newcode38
src/simulator/mpi-interface.cc:38: using namespace std;
On 2009/11/13 20:20:02, craigdo wrote:
> We typically don't do this.

Done.

http://codereview.appspot.com/109068/diff/6010/5070
File src/simulator/simulator.h (right):

http://codereview.appspot.com/109068/diff/6010/5070#newcode618
src/simulator/simulator.h:618: static Ptr<SimulatorImpl>* PeekImpl(void);
On 2009/11/17 14:53:19, Mathieu Lacage wrote:
> what is wrong with SetImplementation and GetImplementation ? These additions
> appear to be redundant with them.

Done.

http://codereview.appspot.com/109068/diff/6010/5072
File wscript (right):

http://codereview.appspot.com/109068/diff/6010/5072#newcode197
wscript:197: opt.add_option('--with-mpi',
On 2009/11/13 20:20:02, craigdo wrote:
> I think this should be --enable-mpi for consistency

Done.

[craigdo1, 2009-12-10 19:05:58]

> -- stability issues.  Craig sent an email about his concerns regarding
> stability in MPI.  I think there was a problem with an example script
> that he created because he was trying to run MPI with non-point-to-
> point
> links.  Currently, this MPI code only works for point-to-point links.
> We will have to stress this in the manual or tutorial when we get to
> that point.

Do you mean that you cannot use CSMA or Wifi at all -- even within the same
rank!?  I would have expected this to be a primary use case.

[riley_ece.gatech.edu, 2009-12-10 19:14:11]

It means that the net devices that directly communicate (sharing a  
single WIFI
or CSMA channel) cannot be split across ranks.  Clearly, we could do a  
distributed
sim with a bunch of separate wifi domains connected via a wired  
infrastructure of
some kind.  The MPI ranks would split across the wired (point to  
point) links.
George
--------------------------------------------------
George Riley
Associate Professor
Georgia Tech
Electrical and Computer Engineering
riley@ece.gatech.edu
404-894-4767
Klaus Advanced Computing Building
Room 3360
266 Ferst Drive
Atlanta, Georgia 30332-0765

ECE6110 Web page:
http://users.ece.gatech.edu/~riley/ece6110/

ECE3090 Web page:
http://users.ece.gatech.edu/~riley/ece3090/





On Dec 10, 2009, at 2:05 PM, <craigdo@ee.washington.edu> wrote:

>> -- stability issues.  Craig sent an email about his concerns  
>> regarding
>> stability in MPI.  I think there was a problem with an example script
>> that he created because he was trying to run MPI with non-point-to-
>> point
>> links.  Currently, this MPI code only works for point-to-point links.
>> We will have to stress this in the manual or tutorial when we get to
>> that point.
>
> Do you mean that you cannot use CSMA or Wifi at all -- even within  
> the same
> rank!?  I would have expected this to be a primary use case.
>
>
>
>
>

[craigdo1, 2009-12-10 19:36:25]

Rearranged responses a little for clarity ...

> >> -- stability issues.  Craig sent an email about his concerns
> >> regarding
> >> stability in MPI.  I think there was a problem with an example
> script
> >> that he created because he was trying to run MPI with non-point-to-
> >> point
> >> links.  Currently, this MPI code only works for point-to-point
> links.

> > Do you mean that you cannot use CSMA or Wifi at all -- even within
> > the same
> > rank!?  I would have expected this to be a primary use case.

> It means that the net devices that directly communicate (sharing a
> single WIFI
> or CSMA channel) cannot be split across ranks.  Clearly, we could do a
> distributed
> sim with a bunch of separate wifi domains connected via a wired
> infrastructure of
> some kind.  The MPI ranks would split across the wired (point to
> point) links.

Okay, that makes more sense.  Your last two sentences describe what I tried.
I took mixed-wireless.cc and changed the wireless backbone (N backbone
routers) to a single MPI point-to-point link and tired to run the result,
which then looked like:

//
//                   rank               :          rank
//                                      :
//          +--------------------------------------------------------+
//          |                 MPI Point to Point Link                |
//          +--------------------------------------------------------+
//                   |                  :                     |
//                   |                  :                     |
//                   |                  :                     |
//               +--------+             :                 +--------+
//     wired LAN | mobile |             :       wired LAN | mobile |
//    -----------| router |             :      -----------| router |
//               +--------+             :                 +--------+
//                   |                  :                     |
//          +----------------+          :            +----------------+
//          |     802.11     |          :            |     802.11     |
//          |   infra net    |          :            |   infra net    |
//          |   K-1 hosts    |          :            |   K-1 hosts    |
//          +----------------+          :            +----------------+
//                                      :
//

I got asserts way down in Buffer which concerned me greatly.

-- Craig

[tomh_tomh.org, 2009-12-11 07:14:23]

joshpelkey@gmail.com wrote:
> Hi all,
> 
> Sorry for the delay after the comments.  We were working on switching
> the serialization API, according to Mathieu's comments.  Since it has
> been a while, I will provide a brief summary of the changes and the
> remaining issues.
> 
> *******
> Changes
> *******
> 
> -- Various small fixes according to the comments (code style, typos,
> etc, which I responded to in the comments at the bottom of this reply)
> 
> -- Buffer API.  Switched the serialization/deserialization methods to
> use a raw buffer.  Added methods to buffer, nix-vector, packet-metadata,
> and packet to support this
> 
> -- Changed distributed-simulator-impl to use simulator-impl rather than
> default-simulator-impl (per Mathieu's comment). In order to do this, we
> introduced a pure virtual method in simulator-impl for GetSystemId.
> This method was added to default-simulator-impl,
> distributed-simulator-impl, and realtime-simulator-impl
> 
> *******
> Issues
> *******
> 
> -- Building with WAF.  I want to do this, and I've tried several times
> to get something to work.  I know it is possible, but I'm just not that
> good at using WAF.  I need to re-start the discussion on ns-developers
> for help with this.
> 
> -- Dependency issues.  Tom and Mathieu commented on dependencies in
> src/simulator/distributed-simulator-impl and/or mpi-interface.  We are
> open to moving these classes wherever they fit best.

Suggest src/distributed or src/mpi (maybe the former in case non-mpi 
distributed code comes into the simulator at some point)

> 
> -- Rank in packet-metadata.  We are still not sure why UID in
> packet-metadata fits, but rank does not.  Tom has pointed out that we
> could get away with taking it out for now, since it isn't used.
> However, in the future, it will be necessary.  

Can you describe where it will be necessary?  If so, can rank be 
higher-order bits of a (enlarged) 64-bit uid?


> http://codereview.appspot.com/109068/diff/6010/5043#newcode53
> examples/mpi/nms-udp-nix.cc:53: #endif
> On 2009/11/16 05:17:31, Tom Henderson wrote:
>> I wonder whether these guards are really necessary in the example
> programs.  Is
>> the goal to make them build even if there is no MPI?
> 
> I think the issue involves mpi.h possibly not existing.  By checking for
> NS3_MPI, we know that the user has enabled MPI, and WAF has checked to
> make sure that they have MPI installed.  The goal was not to build if
> there is no MPI but to not fail if MPI is not enabled.
> 
> You are right though, these examples don't need to be built at all if
> MPI is not enabled.  I imagine this can be handled in the wscript
> somehow.

Yes, I think waf can handle it.

> 
> http://codereview.appspot.com/109068/diff/6010/5043#newcode418
> examples/mpi/nms-udp-nix.cc:418: else if (rank == z%systemCount)
> On 2009/11/16 05:17:31, Tom Henderson wrote:
>> Is there a reason that the example instantiates all objects on all
> systems, and
>> then just turns some of the applications on selectively? Would a
> better example
>> be a program that instead only instantiated the necessary objects on
> each
>> system?
> 
> Yes, the most efficient design would only instantiate objects on the
> appropriate system.  However, for ease-of-use with existing routing
> protocols, the full topology is created on each system.  Only the
> applications are system-specific.  This way a routing protocol like
> global routing can work with MPI.

guidance for users along these lines is probably useful.  If you are not 
trying to run a god's-eye routing protocol, why configure it like that?


> 
> http://codereview.appspot.com/109068/diff/6010/5044#newcode215
> examples/mpi/test-distributed.cc:215: Address
> sinkLocalAddress(InetSocketAddress (Ipv4Address::GetAny (), port));
> On 2009/11/16 05:17:31, Tom Henderson wrote:
>> by the way, I found some stray missing spaces throughout the whole
> patchset.  I
>> wonder whether the GNU indent with -prs option can be used to catch
> these.
> 
> I tried using GNU indent once but failed.  I will look into it again.
> It doesn't look like "prs" is the option though.  That puts a space
> after open parenthesis and before close ( like this ).  I'll try using
> "-gnu --no-tabs".

indent -pcs is the closest I could find, but it messes up templates:
Ptr < Node > ...


> http://codereview.appspot.com/109068/diff/6010/5062
> File src/routing/global-routing/global-route-manager-impl.cc (right):
> 
> http://codereview.appspot.com/109068/diff/6010/5062#newcode552
> src/routing/global-routing/global-route-manager-impl.cc:552: if
> (node->GetSystemId() != MPIInterface::Rank()) continue;
> On 2009/11/16 05:17:31, Tom Henderson wrote:
>> In the example that instantiated every object everywhere and then only
> turned on
>> applications on the relevant system ID, wouldn't global routing still
> work in
>> that case?
> 
>> Need to think about how global routing should interact with MPI.
> Should global
>> routing be extended to handle MPI?  Should it be disabled by default
> when MPI is
>> enabled, forcing user to think about whether it will work for the
> coded
>> scenario?  The above code will cause the global routing to partially
> complete
>> but not return an error.
> 
> Yes, global routing will work with MPI.  Since the full topology is
> created on every logical processor, global routing computes routes for
> any node assigned to the logical processor.  It does not compute routes
> from nodes that are not associated with this logical processor, since
> packets will never originate from these nodes on this LP.

OK

[Mathieu Lacage, 2009-12-14 13:44:33]

josh, george,

Thanks a lot for taking time to address so thoroughly all previous comments: I
have included a couple of very minor comments below.

http://codereview.appspot.com/109068/diff/13046/13058
File src/common/packet.h (right):

http://codereview.appspot.com/109068/diff/13046/13058#newcode36
src/common/packet.h:36: #include "ns3/mpi-interface.h"
un-needed

http://codereview.appspot.com/109068/diff/13046/13058#newcode241
src/common/packet.h:241: Packet (uint32_t mpiRank, uint32_t uid, uint32_t size);
I am not sure I understand who calls this function but I suspect that to
maintain a per-packet rank id, you need the class who creates a packet to do
something special. 

You could get away with this requirement and make it transparent to you and
everyone else if you made the uid allocation code in packet.cc call
Simulator::GetSystemId () and mix it with its 32bit uid to produce a 64bit uid
which is stored in PacketMetadata.

http://codereview.appspot.com/109068/diff/13046/13058#newcode396
src/common/packet.h:396: uint32_t GetMpiRank (void) const;
If you really need a rank identifier for each packet, I think that it would make
sense to follow tom's suggestion, that is:
  - make GetUid return a uint64_t
  - make GetUid return the rank in the top 32 bits of the uid

http://codereview.appspot.com/109068/diff/13046/13071
File src/simulator/mpi-interface.cc (right):

http://codereview.appspot.com/109068/diff/13046/13071#newcode250
src/simulator/mpi-interface.cc:250: Simulator::Schedule (rxTime -
Simulator::Now(),
Call ScheduleWithContext here to avoid the NS_ASSERT in the node.cc code.

http://codereview.appspot.com/109068/diff/13046/13075
File src/simulator/simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/13046/13075#newcode56
src/simulator/simulator-impl.h:56: virtual uint32_t GetSystemId () const = 0;
If you do this, this method should also be exported from simulator.h

[Josh Pelkey, 2009-12-15 17:10:24]

We've changed uid in packet-metadata to a uint64_t.  The rank is now stored in
the upper 32-bits of this uid.  Since we changed the uid in packet-metadata to
64-bit, we also changed the packetUid in the ExtraItem structure in
packet-metadata to 64-bit.  Not 100% sure if this was needed.

http://codereview.appspot.com/109068/diff/13046/13058
File src/common/packet.h (right):

http://codereview.appspot.com/109068/diff/13046/13058#newcode36
src/common/packet.h:36: #include "ns3/mpi-interface.h"
On 2009/12/14 13:44:33, Mathieu Lacage wrote:
> un-needed

Done.

http://codereview.appspot.com/109068/diff/13046/13058#newcode241
src/common/packet.h:241: Packet (uint32_t mpiRank, uint32_t uid, uint32_t size);
On 2009/12/14 13:44:33, Mathieu Lacage wrote:
> I am not sure I understand who calls this function but I suspect that to
> maintain a per-packet rank id, you need the class who creates a packet to do
> something special. 
> 
> You could get away with this requirement and make it transparent to you and
> everyone else if you made the uid allocation code in packet.cc call
> Simulator::GetSystemId () and mix it with its 32bit uid to produce a 64bit uid
> which is stored in PacketMetadata.

I like this idea, but I don't think it will work for what has to be done.  This
new constructor is called after a packet has been sent to another rank and
immediately before packet deserialization.  So if Simulator::GetSystemId () were
called, the wrong rank would be returned, since we want the rank of the
originating simulator.  This information is only contained in the serialized
buffer, which is where we grab it from (along with the original uid) and pass it
to this new constructor.

http://codereview.appspot.com/109068/diff/13046/13058#newcode396
src/common/packet.h:396: uint32_t GetMpiRank (void) const;
On 2009/12/14 13:44:33, Mathieu Lacage wrote:
> If you really need a rank identifier for each packet, I think that it would
make
> sense to follow tom's suggestion, that is:
>   - make GetUid return a uint64_t
>   - make GetUid return the rank in the top 32 bits of the uid

I've removed this method and removed rank from packet-metadata.  It is now
stored in the upper 32-bits as suggested.

http://codereview.appspot.com/109068/diff/13046/13071
File src/simulator/mpi-interface.cc (right):

http://codereview.appspot.com/109068/diff/13046/13071#newcode250
src/simulator/mpi-interface.cc:250: Simulator::Schedule (rxTime -
Simulator::Now(),
On 2009/12/14 13:44:33, Mathieu Lacage wrote:
> Call ScheduleWithContext here to avoid the NS_ASSERT in the node.cc code.

Thank you.  Fixed.

http://codereview.appspot.com/109068/diff/13046/13075
File src/simulator/simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/13046/13075#newcode56
src/simulator/simulator-impl.h:56: virtual uint32_t GetSystemId () const = 0;
On 2009/12/14 13:44:33, Mathieu Lacage wrote:
> If you do this, this method should also be exported from simulator.h

Done.

[Josh Pelkey, 2009-12-15 17:18:45]

http://codereview.appspot.com/109068/diff/19005/20012
File src/common/packet.cc (right):

http://codereview.appspot.com/109068/diff/19005/20012#newcode173
src/common/packet.cc:173: m_metadata (((uint64_t)mpiRank << 32) & uid, size),
Logic typo here :)  Fixed.

[Mathieu Lacage, 2009-12-16 10:39:27]

On Tue, Dec 15, 2009 at 6:10 PM, <joshpelkey@gmail.com> wrote:

>
>  I am not sure I understand who calls this function but I suspect that
>>
> to
>
>> maintain a per-packet rank id, you need the class who creates a packet
>>
> to do
>
>> something special.
>>
>
>  You could get away with this requirement and make it transparent to
>>
> you and
>
>> everyone else if you made the uid allocation code in packet.cc call
>> Simulator::GetSystemId () and mix it with its 32bit uid to produce a
>>
> 64bit uid
>
>> which is stored in PacketMetadata.
>>
>
> I like this idea, but I don't think it will work for what has to be
> done.  This new constructor is called after a packet has been sent to
> another rank and immediately before packet deserialization.  So if
> Simulator::GetSystemId () were called, the wrong rank would be returned,
> since we want the rank of the originating simulator.  This information
> is only contained in the serialized buffer, which is where we grab it
> from (along with the original uid) and pass it to this new constructor.


Presumably, you would create the new buffer from a serialized buffer in this
case with something like this:

Ptr<Packet> p = Create<Packet> ();
p->Deserialize (buffer, size);

In which case you can make Deserialize read the uid from the buffer and
ignore the one it got from the constructor.

Of course, you could also do this:

Ptr<Packet> p = Create<Packet> (buffer, size,
someMagicArgumentToCallTheRightConstructor);

in which case Deserialize is not needed anymore as public API.

You would need to create a special constructor with an extra argument of the
right type to make sure that the above calls your new constructor.


Mathieu
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

[riley_gatech.edu, 2009-12-16 14:01:38]

>
>
> Presumably, you would create the new buffer from a serialized buffer  
> in this case with something like this:
>
> Ptr<Packet> p = Create<Packet> ();
> p->Deserialize (buffer, size);
>
> In which case you can make Deserialize read the uid from the buffer  
> and ignore the one it got from the constructor.
This won't work either, as we don't want UID's "disappearing".   
Ignoring the one automatically
assigned in the constructor uses up one UID.  That process would  
create normal packets
2, 3, 5, 6, ... etc. What happened to 4?  It got used up when  
receiving the MPI packet.

George
>
> Of course, you could also do this:
>
> Ptr<Packet> p = Create<Packet> (buffer, size,  
> someMagicArgumentToCallTheRightConstructor);
>
> in which case Deserialize is not needed anymore as public API.
>
> You would need to create a special constructor with an extra  
> argument of the right type to make sure that the above calls your  
> new constructor.
>
>
> Mathieu
> -- 
> Mathieu Lacage <mathieu.lacage@gmail.com>

[craigdo, 2010-02-24 05:35:28]

Most of my comments aren't very serious.  I still have some significant concerns
about examples.

We have a trivial example, and a ginormous example that seems more like a
performance test.

I would really like to see something along the lines of mixed wireless with the
backbone replaced by a remote point-to-point.  This would show how to actually
deal with not building and executing everything everywhere; and how routing
would have to work when not all nodes are instantiated everywhere.

I think there needs to be a manual section that explains how all of this works
and gives some recommended practices.  I think it needs to explain what happens
when, for example, a user calls pointToPoint.EnablePcapAll() in the various
available configurations.  I don't think the answer is intuitively obvious
(parallel I/O ?? ).

I don't think a lot of this is intuitively obvious.

I still see nix vector pointers in Packet, BTW.  I really don't like that very
much.

http://codereview.appspot.com/109068/diff/31001/31014
File examples/mpi/README (right):

http://codereview.appspot.com/109068/diff/31001/31014#newcode37
examples/mpi/README:37: mpirun -np 2 ./waf --run simple-distributed
How can this work?  ./waf is not a program!

I can do:

  ./waf shell
  cd build/debug/examples/mpi
  mpirun -np 2 simple-distributed

It would be nice if a user could ./waf --mpirun simple-distributed in teh same
way that she does ./waf --pyrun file.py

http://codereview.appspot.com/109068/diff/31001/31015
File examples/mpi/nms-udp-nix.cc (right):

http://codereview.appspot.com/109068/diff/31001/31015#newcode66
examples/mpi/nms-udp-nix.cc:66: main (int argc, char *argv[])
This seems to be a very complicated example.  FWIW, this strikes me as more of a
performance test.

http://codereview.appspot.com/109068/diff/31001/31016
File examples/mpi/simple-distributed.cc (right):

http://codereview.appspot.com/109068/diff/31001/31016#newcode59
examples/mpi/simple-distributed.cc:59: main (int argc, char *argv[])
This script strikes me as being much more complicated than other similar
scripts.  For example, examples/tcp/star.cc makes a star much more easily than
is done here.  Perhpas with some very simple changes to helpers, this could be
made much easier.

http://codereview.appspot.com/109068/diff/31001/31016#newcode88
examples/mpi/simple-distributed.cc:88: cmd.AddValue ("nix", "Toggle the use of
nix-vector or global routing", nix);
What does toggle mean here?  When I think of toggle, I think of turning false
into true or vice versa.  Shouldn't this be something like, "enable nix-vector
routing"?

http://codereview.appspot.com/109068/diff/31001/31016#newcode94
examples/mpi/simple-distributed.cc:94: for (uint32_t i = 0; i < 4; ++i)
Is there a reason you didn't do this in the node container.  It would be trivial
to make a method NodeContainer::Create (n, systemid) to do this for you.

http://codereview.appspot.com/109068/diff/31001/31016#newcode109
examples/mpi/simple-distributed.cc:109: // Create right nodes on left with
I don't understand this comment.

http://codereview.appspot.com/109068/diff/31001/31016#newcode111
examples/mpi/simple-distributed.cc:111: NodeContainer rightLeafNodes;
This would be a lot nicer with a node container Create (4, 1) method

http://codereview.appspot.com/109068/diff/31001/31016#newcode130
examples/mpi/simple-distributed.cc:130: // Add links for left side links
I don't understand this comment.

http://codereview.appspot.com/109068/diff/31001/31020
File src/common/buffer.h (right):

http://codereview.appspot.com/109068/diff/31001/31020#newcode485
src/common/buffer.h:485: * \return zero if buffer not large enough
This needs a method description.

http://codereview.appspot.com/109068/diff/31001/31020#newcode494
src/common/buffer.h:494: * \return zero if a complete buffer is not deserialized
This needs a method description.

http://codereview.appspot.com/109068/diff/31001/31022
File src/common/nix-vector.h (right):

http://codereview.appspot.com/109068/diff/31001/31022#newcode108
src/common/nix-vector.h:108: * \return zero if buffer not large enough
Need method description.

http://codereview.appspot.com/109068/diff/31001/31022#newcode116
src/common/nix-vector.h:116: * \return zero if a complete nix-vector is not
deserialized
Need method description

http://codereview.appspot.com/109068/diff/31001/31024
File src/common/packet-metadata.cc (right):

http://codereview.appspot.com/109068/diff/31001/31024#newcode1268
src/common/packet-metadata.cc:1268: memcpy (current, &data, sizeof (uint8_t));
memcpy one byte?

http://codereview.appspot.com/109068/diff/31001/31024#newcode1280
src/common/packet-metadata.cc:1280: memcpy (current, &data, sizeof (uint16_t));
memcpy two bytes?

http://codereview.appspot.com/109068/diff/31001/31024#newcode1292
src/common/packet-metadata.cc:1292: memcpy (current, &data, sizeof (uint32_t));
memcpy four bytes?

http://codereview.appspot.com/109068/diff/31001/31024#newcode1329
src/common/packet-metadata.cc:1329: memcpy(&data, current, sizeof (uint8_t));
memcpy?

http://codereview.appspot.com/109068/diff/31001/31026
File src/common/packet.cc (right):

http://codereview.appspot.com/109068/diff/31001/31026#newcode23
src/common/packet.cc:23: #include "ns3/simulator.h"
We try to avoid dependencies like this.  Generally simulator lives at a higher
level than common, IMO.  Perhaps the concept of system id needs to go elsewhere?

http://codereview.appspot.com/109068/diff/31001/31026#newcode170
src/common/packet.cc:170: Packet::Packet (uint8_t const*buffer, uint32_t size,
bool magic)
typically written uint8_t const *buffer

http://codereview.appspot.com/109068/diff/31001/31026#newcode547
src/common/packet.cc:547: uint32_t Packet::GetSerializedSize (void) const
Again and again, I have a problem with putting routing-flavor-specific stuff in
Packet.  We have worked quite hard not to just stick stuff like this in Packet. 


IMO, routing information just doesn't belong here and is a horrible precedent. 
I thought we agreed to get rid of this before ns-3.7?

http://codereview.appspot.com/109068/diff/31001/31029
File src/devices/point-to-point/point-to-point-channel.h (right):

http://codereview.appspot.com/109068/diff/31001/31029#newcode99
src/devices/point-to-point/point-to-point-channel.h:99: Time GetDelay (void)
const;
Why is this protected?

http://codereview.appspot.com/109068/diff/31001/31029#newcode105
src/devices/point-to-point/point-to-point-channel.h:105: bool IsInitialized
(void) const;
Why is this protected?

http://codereview.appspot.com/109068/diff/31001/31029#newcode113
src/devices/point-to-point/point-to-point-channel.h:113:
Ptr<PointToPointNetDevice> GetSource (uint32_t i) const;
Why is this protected?

http://codereview.appspot.com/109068/diff/31001/31029#newcode121
src/devices/point-to-point/point-to-point-channel.h:121:
Ptr<PointToPointNetDevice> GetDestination (uint32_t i) const;
Why is this protected?

http://codereview.appspot.com/109068/diff/31001/31034
File src/helper/point-to-point-helper.h (right):

http://codereview.appspot.com/109068/diff/31001/31034#newcode255
src/helper/point-to-point-helper.h:255: ObjectFactory m_remoteChannelFactory;
I think you need to document that remote channels will be automagically created
for you somewher in this file.

http://codereview.appspot.com/109068/diff/31001/31041
File src/routing/global-routing/global-route-manager-impl.cc (right):

http://codereview.appspot.com/109068/diff/31001/31041#newcode703
src/routing/global-routing/global-route-manager-impl.cc:703: if
(node->GetSystemId () != MPIInterface::Rank ()) continue;
I don't really like comparisons like this.  If there is a node->GetSystemId(),
shouldn't we compare it against an MPIInterface::GetSystemId()?

Either the system id should be called rank, or rank should be called system id. 
It hurts my brain to compare system id to rank :-)

http://codereview.appspot.com/109068/diff/31001/31043
File src/simulator/default-simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/31001/31043#newcode60
src/simulator/default-simulator-impl.h:60: virtual uint32_t GetSystemId (void)
const;
Should this really be here?  This doesn't feel like a simulator thing at all. 
It seems to be to be more of a system information thing.  If the mpi interface
is in every simulation, maybe it belongs there or perhaps in common.

[Mathieu Lacage, 2010-02-24 08:03:36]

josh, I am fine with this patchset from the perspective of the code I maintain,
that is, packet and simulator. My main concern is item 3) below which I already
pointed out to you privately but which I could live with for the sake of merging
soon.

A couple of comments to answer craig's comments:

1) I am the one who asked for the use of the simulator API in src/packet.cc. I
agree with you that it's not great but, given the requirement that the packet
uid contains the system id (which is indeed important to debug distributed
simulations), I see no other good way to do this. 

2) I agree that the nix vector stuff is evil/ugly and that it should be fixed
but I don't think we ever agreed to a timeline for fixing it. I don't have
ressources to fix it myself and I agreed to this addition originally (with some
arm twisting) so... Sucks, but I can sort of live with it for now.

3) For the user-visible construction issues, your comment about
NodeContainer/system id is spot-on and mirrors some private comments I did to
josh previously. 

I spent a considerable amount of time to fix the same problem in the
multithreaded simulator and suggested what I think is a better approach to josh
to allow users to construct their distributed simulations and specify the node
id/system id mapping but i did not make that a requirement for merging and josh
obviously does not care enough to fix this issue. 

For the record, here is a copy of my suggested approach to deal with the issue
of assigning a system id to nodes and of keeping track of them and of building
simulation scenarios without having to change user scenarios.

Exerpt from private email:
---------------------------------------------------------
  - remove the system id field from the Node class
  - introduce uint32_t Simulator::GetSystemId (uint32_t context)
  - implement the above trivially for existing simulators by returning
zero all the time
  - implement the above in your mpi simulator by keeping an array of
uint32_t with size = n_nodes where each entry is the systemid associated
to a node id (i.e., each node id is equivalent to a context)
  - add an MpiHelper class:
class MpiHelper
{
public:
  void Install (void) {
    m_sim = ...;
    Simulator::SetImplementation (m_sim);
  }
  // start is the node id of the first node which gets systemid = id
  // end is the node id of the last node + 1 which gets systemid = id
  // id is the system id you want to assign to these nodes
  // basically, [start,end[ is the range of nodes which get systemid=id
  void SetSystemId (uint32_t start, uint32_t end, uint32_t id);
  // load from a text file the node id + system id mapping: I used
personally a very simple format: one line per range, "start-end id"
  void SetSystemId (std::string filename);
private:
  Ptr<MpiSimulator> m_sim;
};

What is the point of the above ? It makes it much easier to modify existing
scripts to make them mpi-aware because you just have to describe your mapping
either at the start of the script file or in a configuration text file or, if
you really want to hack your script, add the calls to SetSystemId in the middle
of your script when you create
the nodes. Alternatively, if the user does not specify any mapping, it's easy to
generate a stupid one just to get started.
------------------------------------------------------------

http://codereview.appspot.com/109068/diff/31001/31024
File src/common/packet-metadata.cc (right):

http://codereview.appspot.com/109068/diff/31001/31024#newcode1352
src/common/packet-metadata.cc:1352: if ((uint32_t)((current + sizeof (uint32_t)
- start)) > maxSize) return 0;
coding style: insert {} + newlines.

[Mathieu Lacage, 2010-02-24 08:03:41]

josh, I am fine with this patchset from the perspective of the code I maintain,
that is, packet and simulator. My main concern is item 3) below which I already
pointed out to you privately but which I could live with for the sake of merging
soon.

A couple of comments to answer craig's comments:

1) I am the one who asked for the use of the simulator API in src/packet.cc. I
agree with you that it's not great but, given the requirement that the packet
uid contains the system id (which is indeed important to debug distributed
simulations), I see no other good way to do this. 

2) I agree that the nix vector stuff is evil/ugly and that it should be fixed
but I don't think we ever agreed to a timeline for fixing it. I don't have
ressources to fix it myself and I agreed to this addition originally (with some
arm twisting) so... Sucks, but I can sort of live with it for now.

3) For the user-visible construction issues, your comment about
NodeContainer/system id is spot-on and mirrors some private comments I did to
josh previously. 

I spent a considerable amount of time to fix the same problem in the
multithreaded simulator and suggested what I think is a better approach to josh
to allow users to construct their distributed simulations and specify the node
id/system id mapping but i did not make that a requirement for merging and josh
obviously does not care enough to fix this issue. 

For the record, here is a copy of my suggested approach to deal with the issue
of assigning a system id to nodes and of keeping track of them and of building
simulation scenarios without having to change user scenarios.

Exerpt from private email:
---------------------------------------------------------
  - remove the system id field from the Node class
  - introduce uint32_t Simulator::GetSystemId (uint32_t context)
  - implement the above trivially for existing simulators by returning
zero all the time
  - implement the above in your mpi simulator by keeping an array of
uint32_t with size = n_nodes where each entry is the systemid associated
to a node id (i.e., each node id is equivalent to a context)
  - add an MpiHelper class:
class MpiHelper
{
public:
  void Install (void) {
    m_sim = ...;
    Simulator::SetImplementation (m_sim);
  }
  // start is the node id of the first node which gets systemid = id
  // end is the node id of the last node + 1 which gets systemid = id
  // id is the system id you want to assign to these nodes
  // basically, [start,end[ is the range of nodes which get systemid=id
  void SetSystemId (uint32_t start, uint32_t end, uint32_t id);
  // load from a text file the node id + system id mapping: I used
personally a very simple format: one line per range, "start-end id"
  void SetSystemId (std::string filename);
private:
  Ptr<MpiSimulator> m_sim;
};

What is the point of the above ? It makes it much easier to modify existing
scripts to make them mpi-aware because you just have to describe your mapping
either at the start of the script file or in a configuration text file or, if
you really want to hack your script, add the calls to SetSystemId in the middle
of your script when you create
the nodes. Alternatively, if the user does not specify any mapping, it's easy to
generate a stupid one just to get started.
------------------------------------------------------------

[craigdo1, 2010-02-24 18:14:24]

[ ... ]

> 2) I agree that the nix vector stuff is evil/ugly and that it should be
> fixed but I don't think we ever agreed to a timeline for fixing it. I
> don't have ressources to fix it myself and I agreed to this addition
> originally (with some arm twisting) so... Sucks, but I can sort of live
> with it for now.

The "plan of record" was to fix it in ns-3.7, as seen below.  I guess I'm
with you:  Sucks, but I can sort of live with it for now.  My current MPI
comments are really more about the lack of follow-through on the nix-vector
status and probably shouldn't affect the MPI submission.  Maybe we should
actually mark SetNixVector and GetNixVector as NS_DEPRECATED as a reminder?

The following was the last word on nix-vectors for ns-3.6, just before it
was pushed:

-----Original Message-----
From: ns-developers-bounces@ISI.EDU [mailto:ns-developers-
bounces@ISI.EDU] On Behalf Of craigdo@ee.washington.edu
Sent: Wednesday, September 16, 2009 1:36 PM
To: ns-developers@ISI.EDU
Subject: [Ns-developers] Last Comments on Merge Queue for Ns-3.6
Release

[ ... ]

For nix-vector routing the proposal is:

Go ahead and use a specific pointer of type Ptr<NixVector> for now in
Packet.  Comment clearly that this is a temporary solution to a specific
problem in a generic class and should not be taken as guidance on how to
solve this kind of problem in the future -- this API essentially goes in as
deprecated.

We file a P2 bug on this (must be fixed by next release) and discuss and
implement a more generic way to associate objects with packets for that next
release, whereupon the specific NixVector Ptr is removed in favor of the new
mechanism.

Nix-Vector Routing can then be merged immediately.

[Josh Pelkey, 2010-02-25 18:13:42]

Thank you all for taking the time to review.  I have tried to address all of the
concerns below.  First I'll respond to the general questions/comments, and then
I will go into the code.  I have uploaded a new patchset here with the changes. 
If you are interested in just a quick look at the changes since the review, you
can see them in mercurial:
http://code.nsnam.org/jpelkey3/ns-3-distributed/rev/27b8157adcbd

On 2010/02/24 05:35:28, craigdo wrote:
> Most of my comments aren't very serious.  I still have some significant
concerns
> about examples.
> 
> We have a trivial example, and a ginormous example that seems more like a
> performance test.
> 
> I would really like to see something along the lines of mixed wireless with
the
> backbone replaced by a remote point-to-point.  This would show how to actually
> deal with not building and executing everything everywhere; and how routing
> would have to work when not all nodes are instantiated everywhere.

I was actually planning on creating a new example soon that has wireless nodes
with the simulation split along a p2p wired backbone (kind of like the simple
dumbbell, but with wireless nodes).  So I will do that this weekend.

> I think there needs to be a manual section that explains how all of this works
> and gives some recommended practices.  I think it needs to explain what
happens
> when, for example, a user calls pointToPoint.EnablePcapAll() in the various
> available configurations.  I don't think the answer is intuitively obvious
> (parallel I/O ?? ).
> 
> I don't think a lot of this is intuitively obvious.

I agree that there should be sections in the manual for this.  I will take out
the README in the examples folder, and start working on manual sections for
distributed simulation.
 
> I still see nix vector pointers in Packet, BTW.  I really don't like that very
> much.

Ok, we can talk about this separately.  As release manager, I have put this down
as another issue to address. Just FYI, I am fine with your recommendation in the
follow up reply.

On 2010/02/24 08:03:41, Mathieu Lacage wrote:
> 3) For the user-visible construction issues, your comment about
> NodeContainer/system id is spot-on and mirrors some private comments I did to
> josh previously. 
> 
> I spent a considerable amount of time to fix the same problem in the
> multithreaded simulator and suggested what I think is a better approach to
josh
> to allow users to construct their distributed simulations and specify the node
> id/system id mapping but i did not make that a requirement for merging and
josh
> obviously does not care enough to fix this issue.

I won't be implementing this for the upcoming release, but I do like the ideas
given.  Thanks again for your help.

http://codereview.appspot.com/109068/diff/31001/31014
File examples/mpi/README (right):

http://codereview.appspot.com/109068/diff/31001/31014#newcode37
examples/mpi/README:37: mpirun -np 2 ./waf --run simple-distributed
On 2010/02/24 05:35:29, craigdo wrote:
> How can this work?  ./waf is not a program!
> 
> I can do:
> 
>   ./waf shell
>   cd build/debug/examples/mpi
>   mpirun -np 2 simple-distributed
> 
> It would be nice if a user could ./waf --mpirun simple-distributed in teh same
> way that she does ./waf --pyrun file.py
> 
> 

I'm not sure, but it works (magic?). To me, using mpirun -np 2 ./waf ... makes
sense from a user perspective.  When you run distributed code using openmpi, you
use mpirun.  When you are doing it for ns-3, you do the same thing and pass in
the ns-3 specific way to run a program.

http://codereview.appspot.com/109068/diff/31001/31015
File examples/mpi/nms-udp-nix.cc (right):

http://codereview.appspot.com/109068/diff/31001/31015#newcode66
examples/mpi/nms-udp-nix.cc:66: main (int argc, char *argv[])
On 2010/02/24 05:35:29, craigdo wrote:
> This seems to be a very complicated example.  FWIW, this strikes me as more of
a
> performance test.

It is complicated, no question.  I have definitely used this as a performance
test for the distributed runs.  I just thought that since it was an example, it
should go here.  I didn't know that we should only put simple examples here. 
Should I delete it?

http://codereview.appspot.com/109068/diff/31001/31016
File examples/mpi/simple-distributed.cc (right):

http://codereview.appspot.com/109068/diff/31001/31016#newcode59
examples/mpi/simple-distributed.cc:59: main (int argc, char *argv[])
On 2010/02/24 05:35:29, craigdo wrote:
> This script strikes me as being much more complicated than other similar
> scripts.  For example, examples/tcp/star.cc makes a star much more easily than
> is done here.  Perhpas with some very simple changes to helpers, this could be
> made much easier.

There is a point-to-point-dumbbell-helper that probably could be modified to
setup a distributed simulation like this.  I liked doing it manually to
explicitly show how to assign system ids.  I think a user could look at this and
get a good idea of how to build up their own custom topology for distributed
simulation.  I don't think it would be as effective if a helper was used.

http://codereview.appspot.com/109068/diff/31001/31016#newcode88
examples/mpi/simple-distributed.cc:88: cmd.AddValue ("nix", "Toggle the use of
nix-vector or global routing", nix);
On 2010/02/24 05:35:29, craigdo wrote:
> What does toggle mean here?  When I think of toggle, I think of turning false
> into true or vice versa.  Shouldn't this be something like, "enable nix-vector
> routing"?

Agree, changed.

http://codereview.appspot.com/109068/diff/31001/31016#newcode94
examples/mpi/simple-distributed.cc:94: for (uint32_t i = 0; i < 4; ++i)
On 2010/02/24 05:35:29, craigdo wrote:
> Is there a reason you didn't do this in the node container.  It would be
trivial
> to make a method NodeContainer::Create (n, systemid) to do this for you.

I like this idea and have implemented it.  I still have the routerNodes (a few
lines down) using the old method, since they are in the same node container but
have different system ids.

http://codereview.appspot.com/109068/diff/31001/31016#newcode109
examples/mpi/simple-distributed.cc:109: // Create right nodes on left with
On 2010/02/24 05:35:29, craigdo wrote:
> I don't understand this comment.

How could you...it doesn't make any sense :) Changed.

http://codereview.appspot.com/109068/diff/31001/31016#newcode111
examples/mpi/simple-distributed.cc:111: NodeContainer rightLeafNodes;
On 2010/02/24 05:35:29, craigdo wrote:
> This would be a lot nicer with a node container Create (4, 1) method

Agree.

http://codereview.appspot.com/109068/diff/31001/31016#newcode130
examples/mpi/simple-distributed.cc:130: // Add links for left side links
On 2010/02/24 05:35:29, craigdo wrote:
> I don't understand this comment.

Fixed.

http://codereview.appspot.com/109068/diff/31001/31020
File src/common/buffer.h (right):

http://codereview.appspot.com/109068/diff/31001/31020#newcode485
src/common/buffer.h:485: * \return zero if buffer not large enough
On 2010/02/24 05:35:29, craigdo wrote:
> This needs a method description.

Done.

http://codereview.appspot.com/109068/diff/31001/31020#newcode494
src/common/buffer.h:494: * \return zero if a complete buffer is not deserialized
On 2010/02/24 05:35:29, craigdo wrote:
> This needs a method description.

Done.

http://codereview.appspot.com/109068/diff/31001/31022
File src/common/nix-vector.h (right):

http://codereview.appspot.com/109068/diff/31001/31022#newcode108
src/common/nix-vector.h:108: * \return zero if buffer not large enough
On 2010/02/24 05:35:29, craigdo wrote:
> Need method description.

Done.

http://codereview.appspot.com/109068/diff/31001/31022#newcode116
src/common/nix-vector.h:116: * \return zero if a complete nix-vector is not
deserialized
On 2010/02/24 05:35:29, craigdo wrote:
> Need method description

Done.

http://codereview.appspot.com/109068/diff/31001/31024
File src/common/packet-metadata.cc (right):

http://codereview.appspot.com/109068/diff/31001/31024#newcode1268
src/common/packet-metadata.cc:1268: memcpy (current, &data, sizeof (uint8_t));
On 2010/02/24 05:35:29, craigdo wrote:
> memcpy one byte?

These methods were created to replace the Read/WriteU8, Read/WriteU16,
Read/WriteU32 calls made when serializing to a Buffer...since we aren't
serializing to Buffer anymore; we are serializing to raw pointer buffer.

http://codereview.appspot.com/109068/diff/31001/31024#newcode1352
src/common/packet-metadata.cc:1352: if ((uint32_t)((current + sizeof (uint32_t)
- start)) > maxSize) return 0;
On 2010/02/24 08:03:36, Mathieu Lacage wrote:
> coding style: insert {} + newlines.

fixed them all here

http://codereview.appspot.com/109068/diff/31001/31026
File src/common/packet.cc (right):

http://codereview.appspot.com/109068/diff/31001/31026#newcode170
src/common/packet.cc:170: Packet::Packet (uint8_t const*buffer, uint32_t size,
bool magic)
On 2010/02/24 05:35:29, craigdo wrote:
> typically written uint8_t const *buffer

Done.

http://codereview.appspot.com/109068/diff/31001/31029
File src/devices/point-to-point/point-to-point-channel.h (right):

http://codereview.appspot.com/109068/diff/31001/31029#newcode99
src/devices/point-to-point/point-to-point-channel.h:99: Time GetDelay (void)
const;
On 2010/02/24 05:35:29, craigdo wrote:
> Why is this protected?

point-to-point-remote-channel derives from this class and utilizes these methods
in its TransmitStart method

http://codereview.appspot.com/109068/diff/31001/31034
File src/helper/point-to-point-helper.h (right):

http://codereview.appspot.com/109068/diff/31001/31034#newcode255
src/helper/point-to-point-helper.h:255: ObjectFactory m_remoteChannelFactory;
On 2010/02/24 05:35:29, craigdo wrote:
> I think you need to document that remote channels will be automagically
created
> for you somewher in this file.

Done.

http://codereview.appspot.com/109068/diff/31001/31041
File src/routing/global-routing/global-route-manager-impl.cc (right):

http://codereview.appspot.com/109068/diff/31001/31041#newcode703
src/routing/global-routing/global-route-manager-impl.cc:703: if
(node->GetSystemId () != MPIInterface::Rank ()) continue;
On 2010/02/24 05:35:29, craigdo wrote:
> I don't really like comparisons like this.  If there is a node->GetSystemId(),
> shouldn't we compare it against an MPIInterface::GetSystemId()?
> 
> Either the system id should be called rank, or rank should be called system
id. 
> It hurts my brain to compare system id to rank :-)

Ok, no problem.  I changed MPIInterface::Rank to MpiInterface::GetSystemId

http://codereview.appspot.com/109068/diff/31001/31043
File src/simulator/default-simulator-impl.h (right):

http://codereview.appspot.com/109068/diff/31001/31043#newcode60
src/simulator/default-simulator-impl.h:60: virtual uint32_t GetSystemId (void)
const;
On 2010/02/24 05:35:29, craigdo wrote:
> Should this really be here?  This doesn't feel like a simulator thing at all. 
> It seems to be to be more of a system information thing.  If the mpi interface
> is in every simulation, maybe it belongs there or perhaps in common.

I think it is normal to associate the system id (or rank) with the simulator. 
If it is not a distributed simulation, then this is just zero.  You can see
where it is used specifically in packet.cc, where it checks the system id in
order to fill in the uid.  I don't know of a better place to put this.

[Josh Pelkey, 2010-03-05 18:37:49]

Hi Craig and Mathieu,

Could either or both of you have a look at the updated patch set?  I think I
addressed all of the previous comments.

FYI, I do plan on working on an MpiHelper as Mathieu has suggested.  I won't
have it for this release though.

Thanks!

--
Josh

[craigdo1, 2010-03-05 21:56:36]

I still see, 

  mpirun -np 2 ./waf --run simple-distributed

This does not work on any of the systems I have tried it on (I use bash and the
system complains that waf is not an executable).
Instead, I have to

  ./waf shell
  cd build/devug/examples/mpi
  mpirun -np2 simple-distributed

You should either document this or add a ./waf --mpirun command.

I still have concerns about how understandable some of this stuff really is to
your average user.  For example, I don't think it is
clear that in the new example, for instance, the wireless code is going to be
running on the processor running the csma network or
vice versa.

I think this submission deserves some kind of tutorial for us dense types that
walks through the implications and explains in more
detail what is happening.  Of course, I think that a lot of our system deserves
some kind of tutorial or book for us dense types.  

That said, I will support merging this submission if you will tell me in your
documentation how I can successfully run at least one
of your examples ;-)

I really encourage you to write up a tutorial on how to use this, if you want
people to start picking it up and running with it.

-- Craig

> -----Original Message-----
> From: ns-3-reviews@googlegroups.com 
> [mailto:ns-3-reviews@googlegroups.com] On Behalf Of 
> joshpelkey@gmail.com
> Sent: Friday, March 05, 2010 10:38 AM
> To: faker.moatamri@gmail.com; craigdo@gmail.com; 
> tomh.org@gmail.com; mathieu.lacage@gmail.com
> Cc: riley@ece.gatech.edu; ns-3-reviews@googlegroups.com; 
> reply@codereview.appspotmail.com
> Subject: Re: Ns-3 MPI Distributed Simulation (issue109068)
> 
> Hi Craig and Mathieu,
> 
> Could either or both of you have a look at the updated patch set?  I
> think I addressed all of the previous comments.
> 
> FYI, I do plan on working on an MpiHelper as Mathieu has suggested.  I
> won't have it for this release though.
> 
> Thanks!
> 
> --
> Josh
> 
> http://codereview.appspot.com/109068/show
> 
> -- 
> You received this message because you are subscribed to the 
> Google Groups "ns-3-reviews" group.
> To post to this group, send email to ns-3-reviews@googlegroups.com.
> To unsubscribe from this group, send email to 
> ns-3-reviews+unsubscribe@googlegroups.com.
> For more options, visit this group at 
> http://groups.google.com/group/ns-3-reviews?hl=en.
> 
> 

[Josh Pelkey, 2010-08-25 17:26:19]

This is a test of the ns-3-reviews post from Rietveld.

[Josh Pelkey, 2010-08-25 18:14:57]

test

[Josh Pelkey, 2010-08-25 18:24:38]

test

[Josh Pelkey, 2010-08-25 18:27:27]

test