Re: Fixed Mirage

[Anil Madhavapeddy <anil@xxxxxxxxxx>]

Hm, this is still pretty broken and crashes immediately on boot with master/:

https://github.com/mirage/mirage-platform/commit/ab88ef636fe3ff06c8638d5ea765ad25bf9610e2

Why did you remove the zeroing of the Io_pages?  That breaks everything and the 
domain crashes as soon as any shared rings are used, since there is garbage on 
the ring at start.  I added a memset(block, 0, len) into alloc_pages, and the 
domain now boots ok, but I still see a lot of packet loss.  Investigating...

-anil

On 11 Jul 2013, at 23:15, Anil Madhavapeddy <anil@xxxxxxxxxx> wrote:

> Awesome! I'm pulling to trunk now and trying this out, as I could reproduce 
> the hanging bug before on a static IP.
> 
> I noticed this:
> https://github.com/mirage/mirage-platform/commit/d22f60048890e0b253788a554d6ce00021f76f4e
> 
> Doesn't this totally nullify the point of Console.log_s blocking?  With the 
> switch to Lwt.async, multiple log_s calls can now interleave and corrupt the 
> console ring (whereas previously they would be synchronous).  This would 
> require a mutex around the console ring to be totally safe (and Console.log 
> exists for when you want to be non-blocking).
> 
> And just a minor thing in the same commit, but the change to "if num > 0" in 
> refill_requests also causes one less event notification, and so isn't quite a 
> noop.  That's worth making a separate commit from the interface adaptation 
> also.
> 
> A good general workflow for making changes is:
> 
> - make many commits and one pull request, and then merge it if you don't want 
> code review (e.g. an urgent fix).  This gives everyone watching the repo an 
> explicit mail about the series.
> 
> - even if you make a lot of changes in one go, split up the commits by using 
> "git add -p" to break up a patch into smaller patches interactively.
> 
> - you will love the smaller commits later on when you are tracking down a 
> bug, since you can use "git bisect" and some shell scripts to automatically 
> narrow it down to one commit in many cases.  This requires slightly more 
> automated unit tests for us.
> 
> -anil
> 
> On 11 Jul 2013, at 21:33, Vincent Bernardoff <vb@xxxxxxxxxxxxxx> wrote:
> 
>> Dear all,
>> 
>> I have pushed patches to resp. mirage-platform and mirage-net that fixes the 
>> DHCP bug (and should fix Balraj issues as well).
>> 
>> The bug was caused by my modification of Io_page: I replaced the function 
>> get_n that was allocating n Bigarrays of size one page by a function that 
>> was allocating one Bigarray of size n pages. I guess that the entire 
>> Bigarray was garbage collected when the first of its page was not referenced 
>> anymore, and that was causing the issue.
>> 
>> Balraj, please could you run your tests with this new version, and tell me 
>> if it works for you ?
>> 
>> At least it fixes DHCP completely.
>> 
>> I broke the UNIX backend temporarily, I will fix it tomorrow.
>> 
>> Good evening,
>> 
>> Vincent
>> 
> 
>