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>