Hello Minchan,
sorry, I don't have enough time at the moment to review it in details
due to some urgent issues I'm working on. can this wait?
I was looking at loop.c awhile ago and was considering to do something
similar to what they have done; but it never happened.
I'm a bit 'surprised' that the performance has just 2x, whilst there
are 4x processing threads. I'd rather expect it to be close to 4x... hm.
On (09/06/16 16:24), Minchan Kim wrote:
[..]
> +static void worker_wake_up(void)
> +{
> + if (workers.nr_running * NR_BATCH_PAGES < workers.nr_req) {
> + int nr_wakeup = (workers.nr_req + NR_BATCH_PAGES) /
> + NR_BATCH_PAGES - workers.nr_running;
> +
> + WARN_ON(!nr_wakeup);
> + wake_up_nr(&workers.req_wait, nr_wakeup);
> }
> +}
this seems to be quite complicated. use a wq perhaps? yes, there is a
common concern with the wq that all of the workers can stall during OOM,
but you already have 2 kmalloc()-s in IO path (when adding a new request),
so low memory condition concerns are out of sight here, I assume.
> +static int __zram_make_async_request(struct zram *zram, struct bio *bio)
> +{
> + int offset;
> + u32 index;
> + struct bio_vec bvec;
> + struct bvec_iter iter;
> + LIST_HEAD(page_list);
> + struct bio_request *bio_req;
> + unsigned int nr_pages = 0;
> + bool write = op_is_write(bio_op(bio));
> +
> + index = bio->bi_iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
> + offset = (bio->bi_iter.bi_sector &
> + (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
> +
> + bio_req = kmalloc(sizeof(*bio_req), GFP_NOIO);
> + if (!bio_req)
> + return 1;
> +
> + /*
> + * Keep bi_vcnt to complete bio handling when all of pages
> + * in the bio are handled.
> + */
> + bio_req->bio = bio;
> + atomic_set(&bio_req->nr_pages, 0);
> +
> + bio_for_each_segment(bvec, bio, iter) {
> + int max_transfer_size = PAGE_SIZE - offset;
> +
> + if (bvec.bv_len > max_transfer_size) {
> + /*
> + * zram_bvec_rw() can only make operation on a single
> + * zram page. Split the bio vector.
> + */
> + struct bio_vec bv;
> +
> + bv.bv_page = bvec.bv_page;
> + bv.bv_len = max_transfer_size;
> + bv.bv_offset = bvec.bv_offset;
> +
> + if (queue_page_request_list(zram, bio_req, &bv,
> + index, offset, write, &page_list))
> + goto out;
> + nr_pages++;
> +
> + bv.bv_len = bvec.bv_len - max_transfer_size;
> + bv.bv_offset += max_transfer_size;
> + if (queue_page_request_list(zram, bio_req, &bv,
> + index + 1, 0, write, &page_list))
> + goto out;
> + nr_pages++;
> + } else
> + if (queue_page_request_list(zram, bio_req, &bvec,
> + index, offset, write, &page_list))
> + goto out;
> + nr_pages++;
^^^^^^^^^^
+ else {
if (queue_page_request_list(zram, bio_req, &bvec...
.....
nr_pages++;
+ }
> + update_position(&index, &offset, &bvec);
> + }
> +
> + run_worker(bio, &page_list, nr_pages);
> + return 0;
> +
> +out:
> + kfree(bio_req);
> +
> + WARN_ON(!list_empty(&page_list));
> + return 1;
> +}
[..]
> +static int create_workers(void)
> +{
> + int i;
> + int nr_cpu = num_online_cpus();
> + struct zram_worker *worker;
> +
> + INIT_LIST_HEAD(&workers.worker_list);
> + INIT_LIST_HEAD(&workers.req_list);
> + spin_lock_init(&workers.req_lock);
> + init_waitqueue_head(&workers.req_wait);
> +
> + for (i = 0; i < nr_cpu; i++) {
> + worker = kmalloc(sizeof(*worker), GFP_KERNEL);
> + if (!worker)
> + goto error;
> +
> + worker->task = kthread_run(zram_thread, NULL, "zramd-%d", i);
there may be several zram devices. may be "zram-%d/%d, device_id, i"
> + if (IS_ERR(worker->task)) {
> + kfree(worker);
> + goto error;
> + }
> +
> + list_add(&worker->list, &workers.worker_list);
> + }
> +
> + return 0;
> +
> +error:
> + destroy_workers();
> + return 1;
> +}
-ss
