> On Feb. 3, 2015, 9:37 p.m., Brandon Potter wrote: > > The patch seems like a landmine for an unsuspecting user as it would be > > difficult to diagnose if swap space is exhausted. It will probably be > > evident that memory exhaustion caused the runtime error (segmentation > > fault), but tracking down the cause to this patch will probably be > > non-trivial (maybe?). For a user, the memory allocation failure from > > insufficient swap space would probably be preferable to a hard to diagnose > > segmentation fault. > > > > Even though it is ugly, maybe it's best to add #if 0 / #endif around the > > code to allow someone to find this later on and enable the feature if they > > need it (with a warning about the segmentation fault). The memory > > allocation failure will point the user to the code in this patch. > > Andreas Hansson wrote: > Fair point. An option would be to not use the flag for "sensible" sizes, > perhaps <16 GB or so, and adopt it for larger sizes with a warning? > > Brandon Potter wrote: > Perhaps that's fine. Although, I think that a value for "sensible" will > vary from person to person. > > Personally, I still feel that it is better to leave it completely > disabled and allow someone to enable it if needed (solely to prevent > confusion later). It is trivial to enable the functionality by modifying the > condition. Another avenue might be whether it is worthwhile to use a > preprocessor define and the #ifdef to enable/disable this during compilation; > might be overkill to go through the trouble though. > > Overall, maybe this is just paranoia on my part; it does seem unlikely to > cause a problem in practice.
The input is much appreciated. My hesitation to the approach of asking the user to enable it if needed, is that it means yet another parameter to keep track of (or yet another patch to manage). If we go for the "limit approach", and a large value like 1 TB perhaps? I also think this is unlikely to cause problems the way it is, and as such I would prefer not adding more configurability unless you really think it is needed. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2624/#review5821 ----------------------------------------------------------- On Feb. 3, 2015, 7:57 p.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2624/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2015, 7:57 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10689:568e9a533cf0 > --------------------------- > mem: mmap the backing store with MAP_NORESERVE > > This patch ensures we can run simulations with very large simulated > memories (at least 64 TB based on some quick runs on a Linux > workstation). In essence this allows us to efficiently deal with > sparse address maps without having to implement a redirection layer in > the backing store. > > This opens up for run-time errors if we eventually exhausts the hosts > memory and swap space, but this should hopefully never happen. > > > Diffs > ----- > > src/mem/physical.cc 7639c17357dc > > Diff: http://reviews.gem5.org/r/2624/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
