On Mon, 22 Sep 2014 09:03:07 +0900 Minchan Kim <minchan@xxxxxxxxxx> wrote:
> Currently, swap_slot_free_notify is used for zram to free
> duplicated copy page for memory efficiency when it knows
> there is no reference to the swap slot.
>
> This patch generalizes it to be able to use for other
> swap hint to communicate with VM.
>
I really think we need to do a better job of documenting the code.
> index 94d93b1f8b53..c262bfbeafa9 100644
> --- a/Documentation/filesystems/Locking
> +++ b/Documentation/filesystems/Locking
> @@ -405,7 +405,7 @@ prototypes:
> void (*unlock_native_capacity) (struct gendisk *);
> int (*revalidate_disk) (struct gendisk *);
> int (*getgeo)(struct block_device *, struct hd_geometry *);
> - void (*swap_slot_free_notify) (struct block_device *, unsigned long);
> + int (*swap_hint) (struct block_device *, unsigned int, void *);
>
> locking rules:
> bd_mutex
> @@ -418,7 +418,7 @@ media_changed: no
> unlock_native_capacity: no
> revalidate_disk: no
> getgeo: no
> -swap_slot_free_notify: no (see below)
> +swap_hint: no (see below)
This didn't tell anyone anythnig much.
> index d78b245bae06..22a37764c409 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -926,7 +926,8 @@ error:
> bio_io_error(bio);
> }
>
> -static void zram_slot_free_notify(struct block_device *bdev,
> +/* this callback is with swap_lock and sometimes page table lock held */
OK, that was useful.
It's called "page_table_lock".
Also *which* page_table_lock? current->mm?
> +static int zram_slot_free_notify(struct block_device *bdev,
> unsigned long index)
> {
> struct zram *zram;
>
> ...
>
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1609,6 +1609,10 @@ static inline bool blk_integrity_is_initialized(struct gendisk *g)
>
> #endif /* CONFIG_BLK_DEV_INTEGRITY */
>
> +enum swap_blk_hint {
> + SWAP_FREE,
> +};
This would be a great place to document SWAP_FREE.
> struct block_device_operations {
> int (*open) (struct block_device *, fmode_t);
> void (*release) (struct gendisk *, fmode_t);
> @@ -1624,8 +1628,7 @@ struct block_device_operations {
> void (*unlock_native_capacity) (struct gendisk *);
> int (*revalidate_disk) (struct gendisk *);
> int (*getgeo)(struct block_device *, struct hd_geometry *);
> - /* this callback is with swap_lock and sometimes page table lock held */
> - void (*swap_slot_free_notify) (struct block_device *, unsigned long);
> + int (*swap_hint)(struct block_device *, unsigned int, void *);
And this would be a suitable place to document ->swap_hint().
- Hint from who to who? Is it the caller providing the callee a hint
or is the caller asking the callee for a hint?
- What is the meaning of the return value?
- What are the meaning of the arguments?
Please don't omit the argument names like this. They are useful! How
is a reader to know what that "unsigned int" and "void *" actually
*do*?
The second arg-which-doesn't-have-a-name should have had type
swap_blk_hint, yes?
swap_blk_hint should be called swap_block_hint. I assume that's what
"blk" means. Why does the name have "block" in there anyway? It has
something to do with disk blocks? How is anyone supposed to work that
out?
->swap_hint was converted to return an `int', but all the callers
simply ignore the return value.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>