Re: Ns-3 MPI Distributed Simulation (issue109068)

[mathieu...@gmail.com]

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

http://codereview.appspot.com/109068

[joshp...@gmail.com]

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.

http://codereview.appspot.com/109068

[Mathieu Lacage]

On Tue, Dec 15, 2009 at 6:10 PM, <joshp...@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...@gmail.com>