| Message ID | 20170405074700.29871-5-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 05-04-17 09:47:00, Vlastimil Babka wrote: > Nandsim has own functions set_memalloc() and clear_memalloc() for robust > setting and clearing of PF_MEMALLOC. Replace them by the new generic helpers. > No functional change. This one smells like an abuser. Why the hell should read/write path touch memory reserves at all! > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Richard Weinberger <richard@nod.at> > --- > drivers/mtd/nand/nandsim.c | 29 +++++++++-------------------- > 1 file changed, 9 insertions(+), 20 deletions(-) > > diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c > index cef818f535ed..03a0d057bf2f 100644 > --- a/drivers/mtd/nand/nandsim.c > +++ b/drivers/mtd/nand/nandsim.c > @@ -40,6 +40,7 @@ > #include <linux/list.h> > #include <linux/random.h> > #include <linux/sched.h> > +#include <linux/sched/mm.h> > #include <linux/fs.h> > #include <linux/pagemap.h> > #include <linux/seq_file.h> > @@ -1368,31 +1369,18 @@ static int get_pages(struct nandsim *ns, struct file *file, size_t count, loff_t > return 0; > } > > -static int set_memalloc(void) > -{ > - if (current->flags & PF_MEMALLOC) > - return 0; > - current->flags |= PF_MEMALLOC; > - return 1; > -} > - > -static void clear_memalloc(int memalloc) > -{ > - if (memalloc) > - current->flags &= ~PF_MEMALLOC; > -} > - > static ssize_t read_file(struct nandsim *ns, struct file *file, void *buf, size_t count, loff_t pos) > { > ssize_t tx; > - int err, memalloc; > + int err; > + unsigned int noreclaim_flag; > > err = get_pages(ns, file, count, pos); > if (err) > return err; > - memalloc = set_memalloc(); > + noreclaim_flag = memalloc_noreclaim_save(); > tx = kernel_read(file, pos, buf, count); > - clear_memalloc(memalloc); > + memalloc_noreclaim_restore(noreclaim_flag); > put_pages(ns); > return tx; > } > @@ -1400,14 +1388,15 @@ static ssize_t read_file(struct nandsim *ns, struct file *file, void *buf, size_ > static ssize_t write_file(struct nandsim *ns, struct file *file, void *buf, size_t count, loff_t pos) > { > ssize_t tx; > - int err, memalloc; > + int err; > + unsigned int noreclaim_flag; > > err = get_pages(ns, file, count, pos); > if (err) > return err; > - memalloc = set_memalloc(); > + noreclaim_flag = memalloc_noreclaim_save(); > tx = kernel_write(file, buf, count, pos); > - clear_memalloc(memalloc); > + memalloc_noreclaim_restore(noreclaim_flag); > put_pages(ns); > return tx; > } > -- > 2.12.2
Michal, Am 05.04.2017 um 13:31 schrieb Michal Hocko: > On Wed 05-04-17 09:47:00, Vlastimil Babka wrote: >> Nandsim has own functions set_memalloc() and clear_memalloc() for robust >> setting and clearing of PF_MEMALLOC. Replace them by the new generic helpers. >> No functional change. > > This one smells like an abuser. Why the hell should read/write path > touch memory reserves at all! Could be. Let's ask Adrian, AFAIK he wrote that code. Adrian, can you please clarify why nandsim needs to play with PF_MEMALLOC? Thanks, //richard
On 04/05/2017 01:36 PM, Richard Weinberger wrote: > Michal, > > Am 05.04.2017 um 13:31 schrieb Michal Hocko: >> On Wed 05-04-17 09:47:00, Vlastimil Babka wrote: >>> Nandsim has own functions set_memalloc() and clear_memalloc() for robust >>> setting and clearing of PF_MEMALLOC. Replace them by the new generic helpers. >>> No functional change. >> >> This one smells like an abuser. Why the hell should read/write path >> touch memory reserves at all! > > Could be. Let's ask Adrian, AFAIK he wrote that code. > Adrian, can you please clarify why nandsim needs to play with PF_MEMALLOC? I was thinking about it and concluded that since the simulator can be used as a block device where reclaimed pages go to, writing the data out is a memalloc operation. Then reading can be called as part of r-m-w cycle, so reading as well. But it would be great if somebody more knowledgeable confirmed this. > Thanks, > //richard >
On Wed 05-04-17 13:39:16, Vlastimil Babka wrote: > On 04/05/2017 01:36 PM, Richard Weinberger wrote: > > Michal, > > > > Am 05.04.2017 um 13:31 schrieb Michal Hocko: > >> On Wed 05-04-17 09:47:00, Vlastimil Babka wrote: > >>> Nandsim has own functions set_memalloc() and clear_memalloc() for robust > >>> setting and clearing of PF_MEMALLOC. Replace them by the new generic helpers. > >>> No functional change. > >> > >> This one smells like an abuser. Why the hell should read/write path > >> touch memory reserves at all! > > > > Could be. Let's ask Adrian, AFAIK he wrote that code. > > Adrian, can you please clarify why nandsim needs to play with PF_MEMALLOC? > > I was thinking about it and concluded that since the simulator can be > used as a block device where reclaimed pages go to, writing the data out > is a memalloc operation. Then reading can be called as part of r-m-w > cycle, so reading as well. But it would be great if somebody more > knowledgeable confirmed this. then this deserves a big fat comment explaining all the details, including how the complete depletion of reserves is prevented.
On 05/04/17 14:39, Vlastimil Babka wrote: > On 04/05/2017 01:36 PM, Richard Weinberger wrote: >> Michal, >> >> Am 05.04.2017 um 13:31 schrieb Michal Hocko: >>> On Wed 05-04-17 09:47:00, Vlastimil Babka wrote: >>>> Nandsim has own functions set_memalloc() and clear_memalloc() for robust >>>> setting and clearing of PF_MEMALLOC. Replace them by the new generic helpers. >>>> No functional change. >>> >>> This one smells like an abuser. Why the hell should read/write path >>> touch memory reserves at all! >> >> Could be. Let's ask Adrian, AFAIK he wrote that code. >> Adrian, can you please clarify why nandsim needs to play with PF_MEMALLOC? > > I was thinking about it and concluded that since the simulator can be > used as a block device where reclaimed pages go to, writing the data out > is a memalloc operation. Then reading can be called as part of r-m-w > cycle, so reading as well. IIRC it was to avoid getting stuck with nandsim waiting on memory reclaim and memory reclaim waiting on nandsim.
On Thu 06-04-17 09:33:44, Adrian Hunter wrote: > On 05/04/17 14:39, Vlastimil Babka wrote: > > On 04/05/2017 01:36 PM, Richard Weinberger wrote: > >> Michal, > >> > >> Am 05.04.2017 um 13:31 schrieb Michal Hocko: > >>> On Wed 05-04-17 09:47:00, Vlastimil Babka wrote: > >>>> Nandsim has own functions set_memalloc() and clear_memalloc() for robust > >>>> setting and clearing of PF_MEMALLOC. Replace them by the new generic helpers. > >>>> No functional change. > >>> > >>> This one smells like an abuser. Why the hell should read/write path > >>> touch memory reserves at all! > >> > >> Could be. Let's ask Adrian, AFAIK he wrote that code. > >> Adrian, can you please clarify why nandsim needs to play with PF_MEMALLOC? > > > > I was thinking about it and concluded that since the simulator can be > > used as a block device where reclaimed pages go to, writing the data out > > is a memalloc operation. Then reading can be called as part of r-m-w > > cycle, so reading as well. > > IIRC it was to avoid getting stuck with nandsim waiting on memory reclaim > and memory reclaim waiting on nandsim. I've got lost in the indirection. Could you describe how would reclaim get stuck waiting on these paths please?
diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c index cef818f535ed..03a0d057bf2f 100644 --- a/drivers/mtd/nand/nandsim.c +++ b/drivers/mtd/nand/nandsim.c @@ -40,6 +40,7 @@ #include <linux/list.h> #include <linux/random.h> #include <linux/sched.h> +#include <linux/sched/mm.h> #include <linux/fs.h> #include <linux/pagemap.h> #include <linux/seq_file.h> @@ -1368,31 +1369,18 @@ static int get_pages(struct nandsim *ns, struct file *file, size_t count, loff_t return 0; } -static int set_memalloc(void) -{ - if (current->flags & PF_MEMALLOC) - return 0; - current->flags |= PF_MEMALLOC; - return 1; -} - -static void clear_memalloc(int memalloc) -{ - if (memalloc) - current->flags &= ~PF_MEMALLOC; -} - static ssize_t read_file(struct nandsim *ns, struct file *file, void *buf, size_t count, loff_t pos) { ssize_t tx; - int err, memalloc; + int err; + unsigned int noreclaim_flag; err = get_pages(ns, file, count, pos); if (err) return err; - memalloc = set_memalloc(); + noreclaim_flag = memalloc_noreclaim_save(); tx = kernel_read(file, pos, buf, count); - clear_memalloc(memalloc); + memalloc_noreclaim_restore(noreclaim_flag); put_pages(ns); return tx; } @@ -1400,14 +1388,15 @@ static ssize_t read_file(struct nandsim *ns, struct file *file, void *buf, size_ static ssize_t write_file(struct nandsim *ns, struct file *file, void *buf, size_t count, loff_t pos) { ssize_t tx; - int err, memalloc; + int err; + unsigned int noreclaim_flag; err = get_pages(ns, file, count, pos); if (err) return err; - memalloc = set_memalloc(); + noreclaim_flag = memalloc_noreclaim_save(); tx = kernel_write(file, buf, count, pos); - clear_memalloc(memalloc); + memalloc_noreclaim_restore(noreclaim_flag); put_pages(ns); return tx; }
Nandsim has own functions set_memalloc() and clear_memalloc() for robust setting and clearing of PF_MEMALLOC. Replace them by the new generic helpers. No functional change. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Cc: Boris Brezillon <boris.brezillon@free-electrons.com> Cc: Richard Weinberger <richard@nod.at> --- drivers/mtd/nand/nandsim.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-)