| Message ID | dea3818d-9780-56b7-c82b-6d93f585c560@linux.vnet.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | external/mambo: support mambo COW mode for PMEM disk | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (53944d45087bd78bf7f9494a54e464c74322a83a) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On Tue, 2020-02-18 at 14:16 -0600, Aaron Sawdey wrote: > I've added support in mambo's "memory mmap" command to have a "cow" mode > which just uses MAP_PRIVATE instead of MAP_SHARED on the file so that writes > to the memory region are not sent back to the file. This allows multiple > mambo instances to share the same filesystem image. > > This is implemented by having a PMEM_MODES environment variable. If this > is set, it is expected to contain a comma separated list of modes (rw or cow) > for the list of files in PMEM_DISK. If there are fewer modes than files, the > remaining files default to "rw". I'm not a big fan of the COW name. It suggests the writes are going somewhere. I realise this is the name used in mambo, but I'm not super keen on it. "PRIVATE" might be a better name. Can we call this (singular) PMEM_MODE since we have PMEM_DISK? Can you also include an example ie export PMEM_DISK="mydisk1, mydisk2" export PMEM_MODE="rw, cow" Other than that, the code looks good. Mikey > > Thanks, > Aaron > > > Signed-off-by: Aaron Sawdey <acsawdey at linux.ibm.com> > diff --git a/external/mambo/skiboot.tcl b/external/mambo/skiboot.tcl > index 8d1cfc66..07c38ab2 100644 > --- a/external/mambo/skiboot.tcl > +++ b/external/mambo/skiboot.tcl > @@ -300,20 +300,31 @@ set pmem_sizes "" > if { [info exists env(PMEM_VOLATILE)] } { > set pmem_sizes [split $env(PMEM_VOLATILE) ","] > } > +set pmem_modes "" > +if { [info exists env(PMEM_MODES)] } { > + set pmem_modes [split $env(PMEM_MODES) ","] > +} > set pmem_root [mysim of addchild $root_node "pmem" ""] > mysim of addprop $pmem_root int "#address-cells" 2 > mysim of addprop $pmem_root int "#size-cells" 2 > mysim of addprop $pmem_root empty "ranges" "" > # Start above where XICS normally is at 0x1A0000000000 > set pmem_start [expr 0x20000000000] > +set pmem_file_ix 0 > foreach pmem_file $pmem_files { # PMEM_DISK > set pmem_file [string trim $pmem_file] > set pmem_size [file size $pmem_file] > - if {[catch {mysim memory mmap $pmem_start $pmem_size $pmem_file rw}]} { > + if { [expr [llength $pmem_modes] > $pmem_file_ix] } { > + set pmem_mode [lindex $pmem_modes $pmem_file_ix] > + } else { > + set pmem_mode "rw" > + } > + if {[catch {mysim memory mmap $pmem_start $pmem_size $pmem_file > $pmem_mode}]} { > puts "ERROR: pmem: 'mysim mmap' command needs newer mambo" > exit > } > set pmem_start [pmem_node_add $pmem_root $pmem_start $pmem_size] > + set pmem_file_ix [expr $pmem_file_ix + 1] > } > foreach pmem_size $pmem_sizes { # PMEM_VOLATILE > set pmem_start [pmem_node_add $pmem_root $pmem_start $pmem_size] >
On Tue, Feb 25, 2020 at 8:15 AM Michael Neuling <mikey@neuling.org> wrote: > > On Tue, 2020-02-18 at 14:16 -0600, Aaron Sawdey wrote: > > I've added support in mambo's "memory mmap" command to have a "cow" mode > > which just uses MAP_PRIVATE instead of MAP_SHARED on the file so that writes > > to the memory region are not sent back to the file. This allows multiple > > mambo instances to share the same filesystem image. > > > > This is implemented by having a PMEM_MODES environment variable. If this > > is set, it is expected to contain a comma separated list of modes (rw or cow) > > for the list of files in PMEM_DISK. If there are fewer modes than files, the > > remaining files default to "rw". > > I'm not a big fan of the COW name. It suggests the writes are going somewhere. I > realise this is the name used in mambo, but I'm not super keen on it. "PRIVATE" > might be a better name. > > Can we call this (singular) PMEM_MODE since we have PMEM_DISK? > > Can you also include an example ie > > export PMEM_DISK="mydisk1, mydisk2" > export PMEM_MODE="rw, cow" > > Other than that, the code looks good. > > Mikey Jokes on you. I already merged it.
On Mon, Feb 24, 2020, at 1:20 PM, Oliver O'Halloran wrote: > On Tue, Feb 25, 2020 at 8:15 AM Michael Neuling <mikey@neuling.org> wrote: > > > > On Tue, 2020-02-18 at 14:16 -0600, Aaron Sawdey wrote: > > > I've added support in mambo's "memory mmap" command to have a "cow" mode > > > which just uses MAP_PRIVATE instead of MAP_SHARED on the file so that writes > > > to the memory region are not sent back to the file. This allows multiple > > > mambo instances to share the same filesystem image. > > > > > > This is implemented by having a PMEM_MODES environment variable. If this > > > is set, it is expected to contain a comma separated list of modes (rw or cow) > > > for the list of files in PMEM_DISK. If there are fewer modes than files, the > > > remaining files default to "rw". > > > > I'm not a big fan of the COW name. It suggests the writes are going somewhere. I > > realise this is the name used in mambo, but I'm not super keen on it. "PRIVATE" > > might be a better name. > > > > Can we call this (singular) PMEM_MODE since we have PMEM_DISK? > > > > Can you also include an example ie > > > > export PMEM_DISK="mydisk1, mydisk2" > > export PMEM_MODE="rw, cow" > > > > Other than that, the code looks good. > > > > Mikey > > Jokes on you. I already merged it. Didn't anyone tell you you're only meant to merge RFC patches? :)
On Tue, Feb 25, 2020 at 2:51 PM Stewart Smith <stewart@flamingspork.com> wrote: > > On Mon, Feb 24, 2020, at 1:20 PM, Oliver O'Halloran wrote: > > On Tue, Feb 25, 2020 at 8:15 AM Michael Neuling <mikey@neuling.org> wrote: > > > > > > On Tue, 2020-02-18 at 14:16 -0600, Aaron Sawdey wrote: > > > > I've added support in mambo's "memory mmap" command to have a "cow" mode > > > > which just uses MAP_PRIVATE instead of MAP_SHARED on the file so that writes > > > > to the memory region are not sent back to the file. This allows multiple > > > > mambo instances to share the same filesystem image. > > > > > > > > This is implemented by having a PMEM_MODES environment variable. If this > > > > is set, it is expected to contain a comma separated list of modes (rw or cow) > > > > for the list of files in PMEM_DISK. If there are fewer modes than files, the > > > > remaining files default to "rw". > > > > > > I'm not a big fan of the COW name. It suggests the writes are going somewhere. I > > > realise this is the name used in mambo, but I'm not super keen on it. "PRIVATE" > > > might be a better name. > > > > > > Can we call this (singular) PMEM_MODE since we have PMEM_DISK? > > > > > > Can you also include an example ie > > > > > > export PMEM_DISK="mydisk1, mydisk2" > > > export PMEM_MODE="rw, cow" > > > > > > Other than that, the code looks good. > > > > > > Mikey > > > > Jokes on you. I already merged it. > > Didn't anyone tell you you're only meant to merge RFC patches? :) Merging RFC patches is only permitted on fridays. > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On Mon, Feb 24, 2020, at 8:07 PM, Oliver O'Halloran wrote: > On Tue, Feb 25, 2020 at 2:51 PM Stewart Smith <stewart@flamingspork.com> wrote: > > > > On Mon, Feb 24, 2020, at 1:20 PM, Oliver O'Halloran wrote: > > > On Tue, Feb 25, 2020 at 8:15 AM Michael Neuling <mikey@neuling.org> wrote: > > > > > > > > On Tue, 2020-02-18 at 14:16 -0600, Aaron Sawdey wrote: > > > > > I've added support in mambo's "memory mmap" command to have a "cow" mode > > > > > which just uses MAP_PRIVATE instead of MAP_SHARED on the file so that writes > > > > > to the memory region are not sent back to the file. This allows multiple > > > > > mambo instances to share the same filesystem image. > > > > > > > > > > This is implemented by having a PMEM_MODES environment variable. If this > > > > > is set, it is expected to contain a comma separated list of modes (rw or cow) > > > > > for the list of files in PMEM_DISK. If there are fewer modes than files, the > > > > > remaining files default to "rw". > > > > > > > > I'm not a big fan of the COW name. It suggests the writes are going somewhere. I > > > > realise this is the name used in mambo, but I'm not super keen on it. "PRIVATE" > > > > might be a better name. > > > > > > > > Can we call this (singular) PMEM_MODE since we have PMEM_DISK? > > > > > > > > Can you also include an example ie > > > > > > > > export PMEM_DISK="mydisk1, mydisk2" > > > > export PMEM_MODE="rw, cow" > > > > > > > > Other than that, the code looks good. > > > > > > > > Mikey > > > > > > Jokes on you. I already merged it. > > > > Didn't anyone tell you you're only meant to merge RFC patches? :) > > Merging RFC patches is only permitted on fridays. I think you mean "after 4pm on Fridays, and only while beer is in hand".
On Wed, Feb 19, 2020 at 10:54 AM Aaron Sawdey <acsawdey@linux.vnet.ibm.com> wrote: > > I've added support in mambo's "memory mmap" command to have a "cow" mode > which just uses MAP_PRIVATE instead of MAP_SHARED on the file so that writes > to the memory region are not sent back to the file. This allows multiple > mambo instances to share the same filesystem image. > > This is implemented by having a PMEM_MODES environment variable. If this > is set, it is expected to contain a comma separated list of modes (rw or cow) > for the list of files in PMEM_DISK. If there are fewer modes than files, the > remaining files default to "rw". > > Thanks, > Aaron Thanks, merged as f123417068e51842004bdc047c8c5107b70442ef I'll happily take a follow up patch for Mikey's suggestions though.
diff --git a/external/mambo/skiboot.tcl b/external/mambo/skiboot.tcl index 8d1cfc66..07c38ab2 100644 --- a/external/mambo/skiboot.tcl +++ b/external/mambo/skiboot.tcl @@ -300,20 +300,31 @@ set pmem_sizes "" if { [info exists env(PMEM_VOLATILE)] } { set pmem_sizes [split $env(PMEM_VOLATILE) ","] } +set pmem_modes "" +if { [info exists env(PMEM_MODES)] } { + set pmem_modes [split $env(PMEM_MODES) ","] +} set pmem_root [mysim of addchild $root_node "pmem" ""] mysim of addprop $pmem_root int "#address-cells" 2 mysim of addprop $pmem_root int "#size-cells" 2 mysim of addprop $pmem_root empty "ranges" "" # Start above where XICS normally is at 0x1A0000000000 set pmem_start [expr 0x20000000000] +set pmem_file_ix 0 foreach pmem_file $pmem_files { # PMEM_DISK set pmem_file [string trim $pmem_file] set pmem_size [file size $pmem_file] - if {[catch {mysim memory mmap $pmem_start $pmem_size $pmem_file rw}]} { + if { [expr [llength $pmem_modes] > $pmem_file_ix] } { + set pmem_mode [lindex $pmem_modes $pmem_file_ix] + } else { + set pmem_mode "rw" + } + if {[catch {mysim memory mmap $pmem_start $pmem_size $pmem_file $pmem_mode}]} { puts "ERROR: pmem: 'mysim mmap' command needs newer mambo" exit } set pmem_start [pmem_node_add $pmem_root $pmem_start $pmem_size] + set pmem_file_ix [expr $pmem_file_ix + 1] } foreach pmem_size $pmem_sizes { # PMEM_VOLATILE set pmem_start [pmem_node_add $pmem_root $pmem_start $pmem_size]
I've added support in mambo's "memory mmap" command to have a "cow" mode which just uses MAP_PRIVATE instead of MAP_SHARED on the file so that writes to the memory region are not sent back to the file. This allows multiple mambo instances to share the same filesystem image. This is implemented by having a PMEM_MODES environment variable. If this is set, it is expected to contain a comma separated list of modes (rw or cow) for the list of files in PMEM_DISK. If there are fewer modes than files, the remaining files default to "rw". Thanks, Aaron Signed-off-by: Aaron Sawdey <acsawdey at linux.ibm.com>