ramzswap: Eliminate stale data from compressed memory (v2)
in [Kernel]
|
Prev: [PATCH 2/3] Add swap slot free callback to block_device_operations
Next: [git pull] more PCMCIA bugfixes for 2.6.34-rc6
From: Pekka Enberg on 7 May 2010 03:50 Nitin Gupta kirjoitti: > (tested on mainline but should apply to linux-next cleanly) > > * Changelog: v2 vs initial patches > - directly add swap free callback to block_device_operations > instead of using 'notifiers' for various swap events. > > ramzswap driver creates RAM based block devices which can be > used (only) as swap disks. Pages swapped to these disks are > compressed and stored in memory itself. > > However, these devices do not get any notification when a swap > slot is freed (swap_map[i] reaches 0). So, we cannot free memory > allocated corresponding to this swap slot. Such stale data can > quickly accumulate in (compressed) memory defeating the whole > purpose of such devices. > > To overcome this problem, we now add a callback in struct > block_device_operations which is called as soon as a swap > slot is freed. > > Nitin Gupta (3): > Add flag to identify block swap devices > Add swap slot free callback to block_device_operations > ramzswap: Handler for swap slot free callback > > drivers/staging/ramzswap/TODO | 5 ----- > drivers/staging/ramzswap/ramzswap_drv.c | 22 +++++++++++++--------- > include/linux/blkdev.h | 2 ++ > include/linux/swap.h | 1 + > mm/swapfile.c | 5 +++++ > 5 files changed, 21 insertions(+), 14 deletions(-) > delete mode 100644 drivers/staging/ramzswap/TODO The series looks good to me: Acked-by: Pekka Enberg <penberg(a)cs.helsinki.fi> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 7 May 2010 11:00 On Fri, 7 May 2010, Pekka Enberg wrote: > > The series looks good to me: > > Acked-by: Pekka Enberg <penberg(a)cs.helsinki.fi> Yeah, I think I'm finally ok with it too. Acked-by: Linus Torvalds <torvalds(a)linux-foundation.org> Thanks, Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Nitin Gupta on 8 May 2010 00:10 On Sat, May 8, 2010 at 1:25 AM, Andrew Morton <akpm(a)linux-foundation.org> wrote: > On Fri, 7 May 2010 12:55:04 +0530 > Nitin Gupta <ngupta(a)vflare.org> wrote: <snip> > > <hasty review> > > Looking at the changelogs I'm seeing no information about the > effectiveness of ramzswap - how much memory it saves. As that's the > entire point of the driver, that would be a rather important thing to > have included in the commit comments. We cannot make the decision to > merge ramzswap without this info. > Documentation (drivers/staging/ramzswap/ramzswap.txt) points to the project home page which has lots of performance numbers -- both positive and negative cases. Lot of your points are regarding lack of documentation and code comments. Instead of replying to all such points individually, let me first send patches with all the code commentary. > The driver appears to be controlled by some nasty-looking ioctl against > some fd. None of it is documented anywhere. It should be. You're > proposing here a permanent extension to the kernel ABI which we will > need to maintain for ever. That's a big deal and it is the very first > thing reviewers will look at, before even considering the code. > ramzswap.txt points to 'rzscontrol' manpage where the effect of each of these ioctls is documented: http://compcache.googlecode.com/hg/sub-projects/rzscontrol/man/rzscontrol.html I will add appropriate comments in code too. > The compat implications of the ioctl args will need to be examined. > Ok, I will look into this. > RZSIO_GET_STATS looks to be hopeless from a long-term maintainability > POV. It's debug code and it would be better to move it into a debugfs > file, where we can then add and remove things at will. > RZSIO_GET_STATS is how we send stats to userspace. Its not some debug code. > The driver appears to create a /dev node called "ramzswap". If so, I > should be able to run mke2fs on it and cheerily use it as a regular > filesystem, should I not? If so then the entire driver is not > swap-specific at all and there should be no mention of "swap" anywhere! > ramzblock would be more appropriate. > You cannot use /dev/ramzswap{0,1,2...} devices for anything other than swap devices since they can only handle page-aligned I/O requests. This restriction highly simplifies the code as handling compression/ decompression for sub-page requests is hard and wasteful. If someone needs a generic in-memory compressed block device, they are welcome to extend ramzswap (or create a new driver). > > I've completely forgotten why we need this xvmalloc thing and I don't > recall whether we decided it would be a good thing to have as a generic > facility and of course it's all unexplained and undocumented. I won't > be looking at it today, for this reason. > Justification for xvmalloc is present in initial commit message (644bf7) > > I'll give up there. > > The overall idea and utility appear to be good and desirable, IMO. But > the code isn't productively reviewable in this state. > I will soon send patches for adding all these code comments. I hope that will make code more reviewable. In the meantime, would it be possible to commit these swap free notify patches? Thanks for your detailed comments. Nitin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Pekka Enberg on 8 May 2010 02:30 Hi Andrew, Andrew Morton wrote: > Looking at the changelogs I'm seeing no information about the > effectiveness of ramzswap - how much memory it saves. As that's the > entire point of the driver, that would be a rather important thing to > have included in the commit comments. We cannot make the decision to > merge ramzswap without this info. There's some benchmarks at ramzswap pages: http://code.google.com/p/compcache/wiki/Performance http://code.google.com/p/compcache/wiki/SwapDiskVsRamz [ snip bunch of comments from Andrew that need to be addressed, hopefully we'll get some help from the staging people ] > The driver appears to be controlled by some nasty-looking ioctl against > some fd. None of it is documented anywhere. It should be. You're > proposing here a permanent extension to the kernel ABI which we will > need to maintain for ever. That's a big deal and it is the very first > thing reviewers will look at, before even considering the code. I thought we got rid of it? Nitin? > RZSIO_GET_STATS looks to be hopeless from a long-term maintainability > POV. It's debug code and it would be better to move it into a debugfs > file, where we can then add and remove things at will. Yup. > I've completely forgotten why we need this xvmalloc thing and I don't > recall whether we decided it would be a good thing to have as a generic > facility and of course it's all unexplained and undocumented. I won't > be looking at it today, for this reason. We need it because the slab allocator is not a good fit for this special purpose driver due to fragmentation. Nitin, you had a nice web page showing all the relevant numbers but I can't find it anymore. Andrew, FWIW, I'm ok with xvmalloc() for this particular driver. There was some discussion on making it more generic but I don't see it as a merge-stopper for the driver. > The overall idea and utility appear to be good and desirable, IMO. But > the code isn't productively reviewable in this state. I agree that the whole graduation step from staging to kernel proper is not well-defined. Any suggestions? That said, I hope that doesn't stop us from merging this patch series because the lack of notifiers cripples the current ramzswap performance. Pekka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Nitin Gupta on 8 May 2010 03:00
Hi, On 05/08/2010 11:59 AM, Pekka Enberg wrote: > > Andrew Morton wrote: >> The driver appears to be controlled by some nasty-looking ioctl against >> some fd. None of it is documented anywhere. It should be. You're >> proposing here a permanent extension to the kernel ABI which we will >> need to maintain for ever. That's a big deal and it is the very first >> thing reviewers will look at, before even considering the code. > > I thought we got rid of it? Nitin? > No, we didn't get rid of any ioctls. In fact, issuing ioctls is how we configure various ramzswap devices (using rzscontrol utility). The functionality of all these ioctls is well documented in rzscontrol manpage. Now, I will also add relevant comments in the code. >> RZSIO_GET_STATS looks to be hopeless from a long-term maintainability >> POV. It's debug code and it would be better to move it into a debugfs >> file, where we can then add and remove things at will. > > Yup. > Its not debug code. This ioctl is how we send stats about given ramzswap device to userspace: rzscontrol /dev/ramzswap --stats, invokes this ioctl and prints the stats. I will add comments to this part to make it clear. Thanks, Nitin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |