On Thu, Oct 25, 2012 at 09:55:39AM +0900, Minchan Kim wrote: > On Mon, Oct 22, 2012 at 10:31:13AM +0800, Shaohua Li wrote: > > swap can do cluster discard for SSD, which is good, but there are some problems > > here: > > 1. swap do the discard just before page reclaim gets a swap entry and writes > > the disk sectors. This is useless for high end SSD, because an overwrite to a > > sector implies a discard to original nand flash too. A discard + overwrite == > > overwrite. > > 2. the purpose of doing discard is to improve SSD firmware garbage collection. > > Doing discard just before write doesn't help, because the interval between > > discard and write is too short. Doing discard async and just after a swap entry > > is freed can make the interval longer, so SSD firmware has more time to do gc. > > 3. block discard is a sync API, which will delay scan_swap_map() significantly. > > 4. Write and discard command can be executed parallel in PCIe SSD. Making > > swap discard async can make execution more efficiently. > > Great! > > > > > This patch makes swap discard async, and move discard to where swap entry is > > freed. Idealy we should do discard for any freed sectors, but some SSD discard > > Yes. It's ideal but most of small storage(ex, eMMC) can't do it due to shortage of > internal resource. > > > is very slow. This patch still does discard for a whole cluster. > > That's good for small nonration storage. > > > > > My test does a several round of 'mmap, write, unmap', which will trigger a lot > > of swap discard. In a fusionio card, with this patch, the test runtime is > > reduced to 18% of the time without it, so around 5.5x faster. > > Could you share your test program? Very simple. for i in $(seq 1 5); do /usr/bin/time -a -o discard-log $USEMEM --prefault 30G done I measure the time of the whole swapout. > > +/* magic number to indicate the cluster is discardable */ > > +#define CLUSTER_COUNT_DISCARDABLE (SWAPFILE_CLUSTER * 2) > > +#define CLUSTER_COUNT_DISCARDING (SWAPFILE_CLUSTER * 2 + 1) > > #define CLUSTER_COUNT_DISCARDING (CLUSTER_COUNT_DISCARDABLE + 1) That's fine, any number above CLUSTER_COUNT_DISCARDABLE is ok. > > +static void swap_cluster_check_discard(struct swap_info_struct *si, > > + unsigned long offset) > > +{ > > + unsigned long cluster = offset/SWAPFILE_CLUSTER; > > + > > + if (!(si->flags & SWP_DISCARDABLE)) > > + return; > > + if (si->swap_cluster_count[cluster] > 0) > > + return; > > + si->swap_cluster_count[cluster] = CLUSTER_COUNT_DISCARDABLE; > > + /* Just mark the swap entries occupied */ > > + memset(si->swap_map + (cluster << SWAPFILE_CLUSTER_SHIFT), > > + SWAP_MAP_BAD, SWAPFILE_CLUSTER); > > You should explain why we need SWAP_MAP_BAD. Ok. > > + schedule_work(&si->discard_work); > > +} > > + > > +static void swap_discard_work(struct work_struct *work) > > +{ > > + struct swap_info_struct *si = container_of(work, > > + struct swap_info_struct, discard_work); > > + unsigned int *counter = si->swap_cluster_count; > > + int i; > > + > > + for (i = round_up(si->cluster_next, SWAPFILE_CLUSTER) / > > Why do we always start si->cluster_next? > IMHO, It would be better to start offset where swap_entry_free free. scan_swap_map() searches from si->cluster_next, my intention is this can let scan_swap_map find cluster easily. Maybe I should add comment here. > > + > > + discard_swap_cluster(si, i << SWAPFILE_CLUSTER_SHIFT, > > + SWAPFILE_CLUSTER); > > + > > + spin_lock(&swap_lock); > > + counter[i] = 0; > > + memset(si->swap_map + (i << SWAPFILE_CLUSTER_SHIFT), > > + 0, SWAPFILE_CLUSTER); > > + spin_unlock(&swap_lock); > > + } > > + } > > +} > > Whole searching for finding discardable cluster is rather overkill if we use > big swap device. > Couldn't we make global discarable cluster counter and loop until it is zero? > Anyway, it's just optimization point and could add up based on this patch. > It shouldn't merge your patch. :) For a 500G swap, this sounds not a big deal. If it's really heavy, we can add a bitmap to speed up the search too. That could be a future patch if we find this is a real problem. Thanks, Shaohua -- 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>