> > >
> > > -/*
> > > +/**
> > > + * sgx_reclaim_epc_pages() - Reclaim EPC pages from the consumers
> > > + * @nr_to_scan: Number of EPC pages to scan for reclaim
> > > + * @ignore_age: Reclaim a page even if it is young
> > > + *
> > > * Take a fixed number of pages from the head of the active page pool
> > > and
> > > * reclaim them to the enclave's private shmem files. Skip the pages,
> > > which have
> > > * been accessed since the last scan. Move those pages to the tail of
> > > active
> > > @@ -292,15 +301,14 @@ static void sgx_reclaimer_write(struct
> > > sgx_epc_page *epc_page,
> > > * problematic as it would increase the lock contention too much,
> > > which would
> > > * halt forward progress.
> > > */
> > > -static void sgx_reclaim_pages(void)
> > > +size_t sgx_reclaim_epc_pages(size_t nr_to_scan, bool ignore_age)
> >
> > 'size_t' looks odd. Any reason to use it?
> >
> > Given you only scan 32 at maximum, seems 'int' is good enough?
> >
>
> Initially was int.
> Jarkko was suggesting ssize_t. I changed to size_t as this function will
> never return negative.
Then 'unsigned int'. We are talking about 32 at max here.
size_t is more suitable for bytes, but we are dealing with number of pages.
Maybe Jarkko could comment why size_t is better.
[...]
> >
> > > i = 0;
> > > list_for_each_entry_safe(epc_page, tmp, &iso, list) {
> > > encl_page = epc_page->encl_page;
> > >
> > > - if (!sgx_reclaimer_age(epc_page))
> > > + if (i == SGX_NR_TO_SCAN_MAX ||
> >
> > i == nr_to_scan?
> >
> Not needed if above for statement fixed for nr_to_scan.
> Anything above MAX will be skipped and put back to LRU.
I believe using nr_to_scan is more logically correct.
[...]
> >
> > I found this function a little bit odd, given the mixing of 'nr_to_scan',
> > SGX_NR_TO_SCAN and SGX_NR_TO_SCAN_MAX.
> >
> > From the changelog:
> >
> > 1) To take a parameter that specifies the number of pages to scan for
> > reclaiming. Define a max value of 32, but scan 16 in the case for the
> > global reclaimer (ksgxd).
> >
> > It appears we want to make this function to scan @nr_to_scan for cgroup,
> > but
> > still want to scan a fixed value for ksgxd, which is SGX_NR_TO_SCAN. And
> > @nr_to_scan can be larger than SGX_NR_TO_SCAN but smaller than
> > SGX_NR_TO_SCAN_MAX.
> >
> > Putting behind the mystery of why above is needed, to achieve it, is it
> > more
> > clear if we do below?
> >
> > int __sgx_reclaim_epc_pages(int nr_to_scan, bool ignore_age)
> > {
> > struct sgx_backing backing[SGX_NR_TO_SCAN_MAX];
> > ...
> >
> > if (nr_to_scan > SGX_NR_TO_SCAN_MAX)
> > return 0;
>
> We could set nr_to_scan to MAX but since this is code internal to driver,
> maybe just make sure callers don't call with bigger numbers.
Please add this check, using WARN_ON_ONCE() if it's better.
Then the code is much easier to review.
>
> >
> > for (i = 0; i < nr_to_scan; i++) {
> > ...
> > }
> >
>
> yes
please fix this up, then ...
>
> > return reclaimed;
> > }
> >
> > /* This is for ksgxd() */
> > int sgx_reclaim_epc_page(void)
> > {
> > return __sgx_reclaim_epc_pages(SGX_NR_TO_SCAN, false);
> > }
>
> Some maintainers may prefer no wrapping.
>
... OK.
