Re: [PATCH v9 08/15] x86/sgx: Implement EPC reclamation flows for cgroup

["Haitao Huang" <haitao.huang@linux.intel.com>]

On Thu, 22 Feb 2024 16:24:47 -0600, Huang, Kai <kai.huang@intel.com> wrote:

>
>
> On 23/02/2024 9:12 am, Haitao Huang wrote:
>> On Wed, 21 Feb 2024 04:48:58 -0600, Huang, Kai <kai.huang@intel.com>  
>> wrote:
>>
>>> On Wed, 2024-02-21 at 00:23 -0600, Haitao Huang wrote:
>>>> StartHi Kai
>>>> On Tue, 20 Feb 2024 03:52:39 -0600, Huang, Kai <kai.huang@intel.com>  
>>>> wrote:
>>>> [...]
>>>> >
>>>> > So you introduced the work/workqueue here but there's no place which
>>>> > actually
>>>> > queues the work.  IMHO you can either:
>>>> >
>>>> > 1) move relevant code change here; or
>>>> > 2) focus on introducing core functions to reclaim certain pages  
>>>> from a
>>>> > given EPC
>>>> > cgroup w/o workqueue and introduce the work/workqueue in later  
>>>> patch.
>>>> >
>>>> > Makes sense?
>>>> >
>>>>
>>>> Starting in v7, I was trying to split the big patch, #10 in v6 as you  
>>>> and
>>>> others suggested. My thought process was to put infrastructure needed  
>>>> for
>>>> per-cgroup reclaim in the front, then turn on per-cgroup reclaim in  
>>>> [v9
>>>> 13/15] in the end.
>>>
>>> That's reasonable for sure.
>>>
>>  Thanks for the confirmation :-)
>>
>>>>
>>>> Before that, all reclaimables are tracked in the global LRU so really
>>>> there is no "reclaim certain pages from a  given EPC cgroup w/o  
>>>> workqueue"
>>>> or reclaim through workqueue before that point, as suggested in #2.  
>>>> This
>>>> patch puts down the implementation for both flows but neither used  
>>>> yet, as
>>>> stated in the commit message.
>>>
>>> I know it's not used yet.  The point is how to split patches to make  
>>> them more
>>> self-contain and easy to review.
>>  I would think this patch already self-contained in that all are  
>> implementation of cgroup reclamation building blocks utilized later.  
>> But I'll try to follow your suggestions below to split further (would  
>> prefer not to merge in general unless there is strong reasons).
>>
>>>
>>> For #2, sorry for not being explicit -- I meant it seems it's more  
>>> reasonable to
>>> split in this way:
>>>
>>> Patch 1)
>>>   a). change to sgx_reclaim_pages();
>>  I'll still prefer this to be a separate patch. It is self-contained  
>> IMHO.
>> We were splitting the original patch because it was too big. I don't  
>> want to merge back unless there is a strong reason.
>>
>>>   b). introduce sgx_epc_cgroup_reclaim_pages();
>>  Ok.
>
> If I got you right, I believe you want to have a cgroup variant function  
> following the same behaviour of the one for global reclaim, i.e., the  
> _current_ sgx_reclaim_pages(), which always tries to scan and reclaim  
> SGX_NR_TO_SCAN pages each time.
>
> And this cgroup variant function, sgx_epc_cgroup_reclaim_pages(), tries  
> to scan and reclaim SGX_NR_TO_SCAN pages each time "_across_ the cgroup  
> and all the descendants".
>
> And you want to implement sgx_epc_cgroup_reclaim_pages() in this way due  
> to WHATEVER reasons.
>
> In that case, the change to sgx_reclaim_pages() and the introduce of  
> sgx_epc_cgroup_reclaim_pages() should really be together because they  
> are completely tied together in terms of implementation.
>
> In this way you can just explain clearly in _ONE_ patch why you choose  
> this implementation, and for reviewer it's also easier to review because  
> we can just discuss in one patch.
>
> Makes sense?
>
>>
>>>   c). introduce sgx_epc_cgroup_reclaim_work_func() (use a better  
>>> name),     which just takes an EPC cgroup as input w/o involving any  
>>> work/workqueue.
>>  This is for the workqueue use only. So I think it'd be better be with  
>> patch #2 below?
>
> There are multiple levels of logic here IMHO:
>
>    1. a) and b) above focus on "each reclaim" a given EPC cgroup
>    2. c) is about a loop of above to bring given cgroup's usage to limit
>    3. workqueue is one (probably best) way to do c) in async way
>    4. the logic where 1) (direct reclaim) and 3) (indirect) are triggered
>
> To me, it's clear 1) should be in one patch as stated above.
>
> Also, to me 3) and 4) are better to be together since they give you a  
> clear view on how the direct/indirect reclaim are triggered.
>
> 2) could be flexible depending on how you see it.  If you prefer viewing  
> it from low-level implementation of reclaiming pages from cgroup, then  
> it's also OK to be together with 1).  If you want to treat it as a part  
> of _async_ way of bring down usage to limit, then _MAYBE_ it's also OK  
> to be with 3) and 4).
>
> But to me 2) can be together with 1) or even a separate patch because  
> it's still kinda of low-level reclaiming details.  3) and 4) shouldn't  
> contain such detail but should focus on how direct/indirect reclaim is  
> done.

I incorporated most of your suggestions, and think it'd be better discuss  
this with actual code.

So I'm sending out v10, and just quickly summarize what I did to address  
this particular issue here.

I pretty much follow above suggestions and end up with two patches:

1) a) and b) above  plus direct reclaim triggered in try_charge() so  
reviewers can see at lease one use of the sgx_cgroup_reclaim_pages(),  
which is the basic building block.

2) All async related: c) above, workqueue, indirect triggered in  
try_charge() which queues the work.

Please review v10 and if you think the triggering parts need be separated  
then I'll separate.

Additionally, after more experimentation, I simplified sgx_reclaim_pages()  
by removing the pointer for *nr_to_scan as you suggested, but returning  
pages collected for isolation (attempted for reclaim) instead of pages  
actually reclaimed. I found performance is acceptable with this approach.

Thanks again for your review.
Haitao