mem: replacing bytesPerCacheLine with DRAM burstLength in SimpleDRAM

Information
Submitter:	Amin Farmahini
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

This patch gets rid of bytesPerCacheLine parameter and makes the DRAM configuration separate from cache line size. 
Instead of bytesPerCacheLine, I define a parameter for DRAM called burst_length. The burst_length parameter shows the length of a DRAM device burst in bits.
Also, I replace lines_per_rowbuffer with device_rowbuffer_size to improve code portablity.

Updates: 
- a burst length in beats for each memory type.
- an interface width for each memory type.
- the memory controller model is extended to reason about "system" packets vs "dram" packets and assemble the responses properly. It means that system packets larger than a full burst are split into multiple dram packets.

Testing Done

Some short and fairly long tests on DDR3 and LPDRR3 with cache lines of 32, 64, and 128 bytes.

Issue Summary

Description	From	Last Updated	Status
Ok, I am confused here. What is the difference between a dram page and a row buffer? It seems to ...	Nilay Vaish	June 17, 2013, 9:59 p.m.	Dropped
Assuming BL stands for burst length, can you explain why do we need this to be fixed to 5ns and ...	Nilay Vaish	June 17, 2013, 10:04 p.m.	Dropped
Now I am confident that some thing is wrong either with the comment here or the one in DDR3's definition.	Nilay Vaish	June 17, 2013, 10:10 p.m.	Dropped
I would rather see an interface width in bits (e.g. 64) and a burst length in "beats", so 8 in ...	Andreas Hansson	June 25, 2013, 9:37 a.m.	Resolved
Would it not make more sense to express the row buffer as a MemorySize param and thus in kb/bytes?	Andreas Hansson	June 29, 2013, 3:43 p.m.	Resolved
I think if we do indeed do this change we should have a clock and do as Nilay suggests. This ...	Andreas Hansson	June 17, 2013, 7:57 a.m.	Open
I think this patch should include the support for splitting cache lines into multiple DRAM bursts. At least the two ...	Andreas Hansson	June 25, 2013, 9:42 a.m.	Resolved
Hi Nilay, You cannot compute this from params as the cache line size is determined in the C++ code.	Andreas Hansson	June 29, 2013, 3:43 p.m.	Dropped
I think we should compute tBURST here, instead of simply assigning a value.	Nilay Vaish	June 25, 2013, 9:41 a.m.	Dropped
Do we need it per device? Why not combined?	Andreas Hansson	June 25, 2013, 9:19 a.m.	Dropped
Same as above, why per device and not per channel? Also, do you not think it is wise to use ...	Andreas Hansson	June 29, 2013, 3:42 p.m.	Resolved
I'd suggest to make all these const	Andreas Hansson	June 25, 2013, 9:13 a.m.	Dropped
I was not referring to the memory size of the system, rather I was thinking to use the parameter type ...	Andreas Hansson	June 29, 2013, 3:42 p.m.	Resolved
I don't see how the bus width and burst size (in bits/bytes) is of any relevance here any longer. Now ...	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
I would change the comment to say, 8 beats across a ...	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Once again, the size in bytes is not as relevant here, it is the BL that matters.	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
...such that we know when the whole packet is served.	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Perhaps name the variable pktCount here as well?	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Same as above	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Could you add a comment that this is called multiple times on the same packet if a system packet is ...	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
unsigned int	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
I still don't understand why this is not const	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
And this one :-)	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
readBursts?	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
I would move this to the initialisation list. Space around '/'	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
\n at the end of the DPRINTF string.	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Move to initialisation list for these two.	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
\n at the end	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Is the packet ptr still needed here?	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Would this not be easy enough to fix here?	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
pktCount > 1, then new BurstHelper here?	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Why involve the absolute address here, and not just divide getSize and burstSize?	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
wrtie	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Once again I'm not sure of the while loop here. Why not a for look with pktCount?	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Alignment here? Worth dealing with that as well I would imagine.	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
dram_pkt->burstHelper still points to the burstHelper.	Andreas Hansson	July 4, 2013, 12:54 a.m.	Open
Can skip the "in bytes".	Andreas Hansson	July 26, 2013, 12:01 a.m.	Resolved
DDR2 -> LPDDR2_S4	Andreas Hansson	July 26, 2013, 12:01 a.m.	Resolved
Perhaps a stupid question, but is this entirely safe? I guess it is...just thinking in terms of types, assumptions on ...	Andreas Hansson	July 24, 2013, 7:28 a.m.	Open
Do you perhaps want to add the device organisation here as well, i.e. 1x32, 8x8 etc.	Andreas Hansson	July 24, 2013, 7:28 a.m.	Open
Not sure we want to count the DRAM bursts here as the stat was primarily for the arrivals. Is totGap ...	Andreas Hansson	July 26, 2013, 12:07 a.m.	Resolved

src/mem/SimpleDRAM.py (Diff revision 1)

Is 'burst length' a standard term for those who dabble with drams? May be the 
comment can be expanded to explain what is meant by a burst.

Amin Farmahini June 17, 2013, 10:35 p.m. (June 17, 2013, 10:35 p.m.)
```
Yes, it is.
```

src/mem/SimpleDRAM.py (Diff revision 1)

Ok, I am confused here. What is the difference between a dram page and a 
row buffer? It seems to me that you are assuming something more than what 
has been stated here as 128 * 64 != 1024.

Show all issues

Amin Farmahini June 17, 2013, 10:35 p.m. (June 17, 2013, 10:35 p.m.)

as far as I know, row and page are usually used the same way. I am saying usually, because some people tend to use differently. Andreas may correct me.
1kB per module. since this is 8x8, so the rowbuffer is 8k per entire DRAM.

Nilay Vaish June 19, 2013, 2:57 a.m. (June 19, 2013, 2:57 a.m.)

Then, mention this in that comment. I think you are missing the fact that
for three types of DRAM defined, all of them have almost the same comment
before bursts_per_rowbuffer is assigned.

Second, since this param is being computed from other params, therefore
either this should not be a param at all. Or it should be at least computed
from those otehr params and not directly assigned like this.

src/mem/SimpleDRAM.py (Diff revision 1)

Assuming BL stands for burst length, can you explain why do we need this 
to be fixed to 5ns and not make it a function of the burst length? 
Secondly, why is this not a function of the clock frequency?

Show all issues

Amin Farmahini June 17, 2013, 10:35 p.m. (June 17, 2013, 10:35 p.m.)

DDR3 supports only BL8.
DDR2 supports both BL4 and BL8.
so tBURST is fixed for DDR3.
well, it is a function of freq, 1.25*ns4=5ns. note that burst length is always 8 for DDR3. I did not change it, because right now DRAM parameters are not defined to be a function of DRAM internal freq and DRAM bus frequency.

src/mem/SimpleDRAM.py (Diff revision 1)

Now I am confident that some thing is wrong either with the comment 
here or the one in DDR3's definition.

Show all issues

Amin Farmahini June 17, 2013, 10:35 p.m. (June 17, 2013, 10:35 p.m.)

not wrong. this is a 1x32 configuration, so there is only one module. so 16*64=1kB. Up there, there were 8 modules.

src/mem/simple_dram.cc (Diff revision 1)

Typo in length. Actually, I was not aware that you can write comments after the if() statement without using braces.

Amin Farmahini June 17, 2013, 10:35 p.m. (June 17, 2013, 10:35 p.m.)
```
we'll fixit.
```

src/mem/simple_dram.cc (Diff revision 1)

Looking at how it is incremented, this was really shady.

Amin Farmahini June 17, 2013, 10:35 p.m. (June 17, 2013, 10:35 p.m.)

yeah, actually I did not think that was correct (or at least it could be wrong in many cases). so I removed it.

src/mem/SimpleDRAM.py (Diff revision 1)

I would rather see an interface width in bits (e.g. 64) and a burst length in "beats", so 8 in this case.

Show all issues

Amin Farmahini June 17, 2013, 10:34 p.m. (June 17, 2013, 10:34 p.m.)

yes, that would be better. 
So, I'll add an interface width in bits and burst length in beats for each DRAM. 
I'll try to do it on June 30th (can't before that).

src/mem/SimpleDRAM.py (Diff revision 1)

Would it not make more sense to express the row buffer as a MemorySize param and thus in kb/bytes?

Show all issues

Amin Farmahini June 17, 2013, 10:34 p.m. (June 17, 2013, 10:34 p.m.)

yes, that's what I suggested in my email too. 
the reason that I did not do that is that I did not want to divert from your design significantly. I knew you guys will raise a lot of questions, so I tried to stick with the original code as much as I can, so I get fewer questions :).
Now that you want it, sure I'll do it.

Andreas Hansson June 18, 2013, 9:53 a.m. (June 18, 2013, 9:53 a.m.)
```
Excellent. Great initiative by the way :-)
```

src/mem/SimpleDRAM.py (Diff revision 1)

I think if we do indeed do this change we should have a clock and do as Nilay suggests. This also opens up the complication that the WideIO config is SDR and the others are DDR so that has to be captured somehow.

Show all issues

Amin Farmahini June 17, 2013, 10:34 p.m. (June 17, 2013, 10:34 p.m.)

it is good to have clocks involved here, but I think it adds unnecessary complexity.

src/mem/simple_dram.cc (Diff revision 1)

I think this patch should include the support for splitting cache lines into multiple DRAM bursts. At least the two patches should be committed as one.

Show all issues

Amin Farmahini June 17, 2013, 10:34 p.m. (June 17, 2013, 10:34 p.m.)

Some notes here. The current DRAM controller models transaction queues, but it does not model a DRAM command queue. As far as I know, when a dram controller gets a request that should be split into multiple bursts, it keeps it as a single transaction, but the number of associated dram commands increases linearly with the number of bursts. Since, we don't have a command queue, I could not think of a simple way to do this (by simple I mean something that can be done in an hour). I am not a DRAM guru, and I was told this is how dram controller takes care of multiple burst requests. So, if you have any suggestions/comments here, I would love to know.

src/mem/SimpleDRAM.py (Diff revision 1)

Hi Nilay,

You cannot compute this from params as the cache line size is determined in the C++ code.

Show all issues

Change Summary:

Updates: 
- a burst length in beats for each memory type.
- an interface width for each memory type.
- the memory controller model is extended to reason about "system" packets vs "dram" packets and assemble the responses properly. It means that system packets larger than a full burst are split into multiple dram packets.

Diff:

Revision 2 (+355 -182)

Show changes

	src/mem/SimpleDRAM.py
	src/mem/simple_dram.hh
	src/mem/simple_dram.cc

Description:

		This patch gets rid of bytesPerCacheLine parameter and makes the DRAM configuration separate from cache line size.
~		Instead of bytesPerCacheLine, I define a parameter for DRAM called burst_length. The burst_length parameter shows the size of a DRAM burst in bytes and is 64 bytes for all current DRAM configurations. Note, this parameter is coupled with tBURST.
~		Also, I replace lines_per_rowbuffer with bursts_per_rowbuffer, because lines_per_rowbuffer is defined based on 64-byte cache lines which makes the code unportable.
	~	Instead of bytesPerCacheLine, I define a parameter for DRAM called burst_length. The burst_length parameter shows the length of a DRAM device burst in bits.
	~	Also, I replace lines_per_rowbuffer with device_rowbuffer_size to improve code portablity.

~		Next patch could be to add support for requests larger than burst length.
	~	Updates:
	+	- a burst length in beats for each memory type.
	+	- an interface width for each memory type.
	+	- the memory controller model is extended to reason about "system" packets vs "dram" packets and assemble the responses properly. It means that system packets larger than a full burst are split into multiple dram packets.

src/mem/SimpleDRAM.py (Diff revision 2)

I think we should compute tBURST here, instead of simply assigning a value.

Show all issues

Amin Farmahini June 24, 2013, 11:30 a.m. (June 24, 2013, 11:30 a.m.)

This could be done, but I believe this should be done in another patch because it is not directly related to what this patch is trying to do. Also, this patch is a pretty big change by itself. Let's submit this patch and then next step could be your suggestion (which is adding bus freq and DRAM freq and make some parameters such as tBURST a function of freq).

Overall this looks like a great step in the right direction. One thing that struck me is the complexity of the iteration to split the packets. Why make it so complicated? Would we not simply change decode packet to take a container as an input parameter and push back as many packets as needed? This should also help remove any assumptions on alignment. What do you think?

Amin Farmahini June 25, 2013, 9:37 a.m. (June 25, 2013, 9:37 a.m.)

I read your comment multiple times to grasp it, but still not sure I fully understand it.

"change decode packet to take a container as an input parameter and push back as many packets as needed". before pushing each split packet, I make sure this split packet is not in the write queue. If it is in the write queue, we don't push it back. If we push all split packets at once, there is no way to check the write queue. Please, note that a packet could be split into multiple split burst-size packets in which some burst-size packets could be in the write queue and some not. 

Overall, I am open to discuss it further and willing to make it more simple, if possible.

Andreas Hansson June 27, 2013, 10:36 a.m. (June 27, 2013, 10:36 a.m.)

Good point about the write queue. I was thinking something like:

decode(PacketPtr, queue&)

Where we would decode the packet and add it to the queue. As you point out, this will be tricky with the write queue and we need to check if there is already a hit of partial hit.

src/mem/SimpleDRAM.py (Diff revision 2)

Do we need it per device? Why not combined?

Show all issues

Amin Farmahini June 25, 2013, 9:36 a.m. (June 25, 2013, 9:36 a.m.)

I believe this way is more clear. we use devicesPerRank to calculate the combined bus width. (devicesPerRank * deviceBusWidth)
The devicesPerRank parameter helps us find out how many devices we have. As in our current configurations, some DRAMs have 8 devices, and some only 1. It is better to distinguish between them and make it more clear how many devices we have.

Andreas Hansson June 27, 2013, 10:36 a.m. (June 27, 2013, 10:36 a.m.)

Agreed. From a power modelling point of view we also need the device count.

src/mem/SimpleDRAM.py (Diff revision 2)

Same as above, why per device and not per channel?

Also, do you not think it is wise to use the Param.MemorySize here and specify it in bytes of kb?

Show all issues

Amin Farmahini June 25, 2013, 9:36 a.m. (June 25, 2013, 9:36 a.m.)

similarly, we use devicesPerRank to calculate the total row buffer size => (simple_dram.cc:108) rowBufferSize = devicesPerRank * deviceRowBufferSize;
The reason is clarity again. 

For your other question, so do you mean we want to have a fixed memory size?

src/mem/simple_dram.hh (Diff revision 2)

I'd suggest to make all these const

Show all issues

Amin Farmahini June 25, 2013, 9:36 a.m. (June 25, 2013, 9:36 a.m.)

Please note that burst_length and burst_size are different. burst_length is defined const, but burst_size is not const because burst_size is initialized in the body just like rowsPerBank.

src/mem/SimpleDRAM.py (Diff revision 2)

I was not referring to the memory size of the system, rather I was thinking to use the parameter type Param.MemorySize. What do you think?

Show all issues

Amin Farmahini June 27, 2013, 10:54 a.m. (June 27, 2013, 10:54 a.m.)

Sorry. I don't understand. Would you explain it a bit more? 
Are you saying to use parameter type Param.MemorySize to calculate rowbufferSize?

Andreas Hansson June 27, 2013, 3:39 p.m. (June 27, 2013, 3:39 p.m.)

Not as much calculate as specify. Do you not think that makes more sense than the implicit unit (bytes)?

Amin Farmahini June 27, 2013, 4:20 p.m. (June 27, 2013, 4:20 p.m.)

Now I got it :). I do agree and will fix it.
I think this is the last remaining issue. Please let me know if there is any thing else. If nothing's left, I'll fix this and will submit the new patch soon.

Change Summary:

Use Param.MemorySize for device_rowbuffer_size.

Diff:

Revision 3 (+355 -182)

Show changes

	src/mem/SimpleDRAM.py
	src/mem/simple_dram.hh
	src/mem/simple_dram.cc

src/mem/SimpleDRAM.py (Diff revision 3)

I don't see how the bus width and burst size (in bits/bytes) is of any relevance here any longer. Now it is all about the burst length (in beats), is it not?

Show all issues

src/mem/SimpleDRAM.py (Diff revision 3)

I would change the comment to say, 8 beats across a ...

Show all issues

src/mem/SimpleDRAM.py (Diff revision 3)

Once again, the size in bytes is not as relevant here, it is the BL that matters.

Show all issues

src/mem/simple_dram.hh (Diff revision 3)

...such that we know when the whole packet is served.

Show all issues

src/mem/simple_dram.hh (Diff revision 3)

Perhaps name the variable pktCount here as well?

Show all issues

src/mem/simple_dram.hh (Diff revision 3)

Same as above

Show all issues

src/mem/simple_dram.hh (Diff revision 3)

Could you add a comment that this is called multiple times on the same packet if a system packet is larger than the burst of the memory and that the addr is used for the offset within that packet.

Show all issues

src/mem/simple_dram.hh (Diff revision 3)

unsigned int

Show all issues

src/mem/simple_dram.hh (Diff revision 3)

I still don't understand why this is not const

Show all issues

src/mem/simple_dram.hh (Diff revision 3)

And this one :-)

Show all issues

src/mem/simple_dram.hh (Diff revision 3)

readBursts?

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

I would move this to the initialisation list. Space around '/'

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

\n at the end of the DPRINTF string.

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

Move to initialisation list for these two.

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

\n at the end

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

Is the packet ptr still needed here?

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

Would this not be easy enough to fix here?

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

pktCount > 1, then new BurstHelper here?

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

Why involve the absolute address here, and not just divide getSize and burstSize?

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

wrtie

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

Once again I'm not sure of the while loop here. Why not a for look with pktCount?

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

Alignment here? Worth dealing with that as well I would imagine.

Show all issues

src/mem/simple_dram.cc (Diff revision 3)

dram_pkt->burstHelper still points to the burstHelper.

Show all issues

Once the last little bits are fixed I'll add this to the regressions and make sure everything passes before pushing it out.

Andreas Hansson July 18, 2013, 6:21 a.m. (July 18, 2013, 6:21 a.m.)

Did you get a chance to fix the last bits Amin? It would be great if we could get this out. Thanks.

Amin Farmahini July 18, 2013, 8:33 a.m. (July 18, 2013, 8:33 a.m.)
```
It's on my list, hopefully this weekend.
```

Change Summary:

Diff:

Revision 4 (+376 -191)

Show changes

	src/mem/SimpleDRAM.py
	src/mem/simple_dram.hh
	src/mem/simple_dram.cc

Some very minor comments, then it's good to go.

Have you run a complete regression with this patch? I'll kick one off just in case.

src/mem/SimpleDRAM.py (Diff revision 4)

Can skip the "in bytes".

Show all issues

src/mem/SimpleDRAM.py (Diff revision 4)

DDR2 -> LPDDR2_S4

Show all issues

src/mem/simple_dram.cc (Diff revision 4)

Perhaps a stupid question, but is this entirely safe? I guess it is...just thinking in terms of types, assumptions on powers of two etc.

Show all issues

Andreas Hansson July 24, 2013, 9:40 a.m. (July 24, 2013, 9:40 a.m.)

I ran the quick regressions and it seems at least the readPktSize stat changes significantly even with a DDR3 that has a 64 byte burst size. I haven't yet figured out why, but it would be good to understand better why the masking changes the stats.

src/mem/simple_dram.cc (Diff revision 4)

Do you perhaps want to add the device organisation here as well, i.e. 1x32, 8x8 etc.

Show all issues

src/mem/simple_dram.cc (Diff revision 4)

Not sure we want to count the DRAM bursts here as the stat was primarily for the arrivals. Is totGap not for the "system" packets any longer?

Show all issues

Amin Farmahini July 26, 2013, 12:07 a.m. (July 26, 2013, 12:07 a.m.)
```
I fixed this too. My bad.
```

Testing Done:

~		None
	~	Some short and fairly long tests on DDR3 and LPDRR3 with cache lines of 32, 64, and 128 bytes.

Diff:

Revision 5 (+377 -190)

Show changes

	src/mem/SimpleDRAM.py
	src/mem/simple_dram.hh
	src/mem/simple_dram.cc

I'm still seeing quite some stats differences for e.g.: build/ARM/tests/opt/quick/fs/10.linux-boot/arm/linux/realview-simple-timing

I am not sure why and I'd like to understand before pushing this out. The regressions all use the DDR3 config with a 64 byte burst size, so I do not see why the behaviuor should change. Amin, any ideas?

Andreas Hansson July 29, 2013, 7:06 a.m. (July 29, 2013, 7:06 a.m.)

My initial guess is that this is due to accesses that are unaligned and were previously snooped in the write buffer regardless. The "fix" would be to do write merging in the buffer, but let's leave that for a later patch.

Amin Farmahini July 29, 2013, 10:28 a.m. (July 29, 2013, 10:28 a.m.)

I took a quick look at the stats diff and this is what I think is going on:
In the new patch, before adding a packet to the read q, we check its address and its SIZE against packets in the write q. See line 312 in the patched simple_dram.cc. Previously, we did not check whether the size is matching or not, which is clearly wrong. In the new patch, read packets are serviced by wr q only if their size is equal or smaller than a write packet in the wr q. 
Now lets consider build/ARM/tests/opt/quick/fs/10.linux-boot/arm/linux/realview-simple-timing. In this case, most of read packets are 8 bytes, and most of write packets are 4 bytes. So those read packets should NOT find a match in wr q. That explains wr q behavior for most of packets, but for other packets such 64-byte read/write packets and 4-byte read packets, one should dig in further.

Amin Farmahini Aug. 6, 2013, 10:46 a.m. (Aug. 6, 2013, 10:46 a.m.)

Is this patch good to go? If any other changes are needed, please let me know.

Andreas Hansson Aug. 6, 2013, 10:53 a.m. (Aug. 6, 2013, 10:53 a.m.)

The short answer is yes. I've got it ready to go out. I merely want to reduce the size of changesets due to stats differences and so I'd like to push this out together with the write merging (and both of their stats updates). I hope that's ok.

Change Summary:

Fixes a bug in addToReadQueue(). 
The bug showed up when the read packet is larger than DRAM burst size AND the first read burst hits in the write queue. Subsequent read bursts did not get the correct starting addr.
Sorry, My bad

Diff:

Revision 6 (+377 -190)

Show changes

	src/mem/SimpleDRAM.py
	src/mem/simple_dram.hh
	src/mem/simple_dram.cc

Could you mark this submitted Amin?

Could you mark this submitted Amin?

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Change Summary:

Diff:

Description:

Change Summary:

Diff:

Change Summary:

Diff:

Testing Done:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)