[-next] mm: zswap: update comment for struct zswap_entry

Commit Message

Xiu Jianfeng Aug. 8, 2023, 2:47 a.m. UTC

From: Xiu Jianfeng <[e-mail]>

Since commit 0bb488498c98 ("mm: zswap: remove zswap_header"), the
'offset' has been replaced by swpentry, update the comment for it,
and also add comment for 'objcg'.

Signed-off-by: Xiu Jianfeng <[e-mail]>
---
 mm/zswap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Yosry Ahmed Aug. 8, 2023, 5:59 a.m. UTC | #1

Hi Xiu,

On Mon, Aug 7, 2023 at 7:49 PM Xiu Jianfeng <[e-mail]> wrote:
>
> From: Xiu Jianfeng <[e-mail]>
>
> Since commit 0bb488498c98 ("mm: zswap: remove zswap_header"), the
> 'offset' has been replaced by swpentry, update the comment for it,
> and also add comment for 'objcg'.
>
> Signed-off-by: Xiu Jianfeng <[e-mail]>
> ---
>  mm/zswap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 5b56d38e7339..d7863c139c82 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -182,7 +182,7 @@ struct zswap_pool {
>   * page within zswap.
>   *
>   * rbnode - links the entry into red-black tree for the appropriate swap type
> - * offset - the swap offset for the entry.  Index into the red-black tree.
> + * swpentry - the swap entry for the entry, include 'type' and 'offset' fields


This is not the place to document that a swap entry has 'type' and
'offset' fields.

I would also avoid the repetitive 'entry'. Maybe something like:
swpentry - associated swap entry, the offset indexes into the red-black tree

>
>   * refcount - the number of outstanding reference to the entry. This is needed
>   *            to protect against premature freeing of the entry by code
>   *            concurrent calls to load, invalidate, and writeback.  The lock
> @@ -195,6 +195,7 @@ struct zswap_pool {
>   * pool - the zswap_pool the entry's data is in
>   * handle - zpool allocation handle that stores the compressed page data
>   * value - value of the same-value filled pages which have same content
> + * objcg - the obj_cgroup(actually memcg) this entry belongs to


Again, not the place to document that objcg is an abstraction layer
above a memcg.

I would do something like:
objcg - the obj_cgroup that the compressed memory is charged to

>
>   * lru - handle to the pool's lru used to evict pages.
>   */
>  struct zswap_entry {
> --
> 2.34.1
>
>

Xiu Jianfeng Aug. 8, 2023, 6:26 a.m. UTC | #2

Hi Yosry,

On 2023/8/8 13:59, Yosry Ahmed wrote:
> Hi Xiu,
> 
> On Mon, Aug 7, 2023 at 7:49 PM Xiu Jianfeng <[e-mail]> wrote:
>>
>> From: Xiu Jianfeng <[e-mail]>
>>
>> Since commit 0bb488498c98 ("mm: zswap: remove zswap_header"), the
>> 'offset' has been replaced by swpentry, update the comment for it,
>> and also add comment for 'objcg'.
>>
>> Signed-off-by: Xiu Jianfeng <[e-mail]>
>> ---
>>  mm/zswap.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index 5b56d38e7339..d7863c139c82 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -182,7 +182,7 @@ struct zswap_pool {
>>   * page within zswap.
>>   *
>>   * rbnode - links the entry into red-black tree for the appropriate swap type
>> - * offset - the swap offset for the entry.  Index into the red-black tree.
>> + * swpentry - the swap entry for the entry, include 'type' and 'offset' fields
> 
> 
> This is not the place to document that a swap entry has 'type' and
> 'offset' fields.
> 
> I would also avoid the repetitive 'entry'. Maybe something like:
> swpentry - associated swap entry, the offset indexes into the red-black tree
> 
>>
>>   * refcount - the number of outstanding reference to the entry. This is needed
>>   *            to protect against premature freeing of the entry by code
>>   *            concurrent calls to load, invalidate, and writeback.  The lock
>> @@ -195,6 +195,7 @@ struct zswap_pool {
>>   * pool - the zswap_pool the entry's data is in
>>   * handle - zpool allocation handle that stores the compressed page data
>>   * value - value of the same-value filled pages which have same content
>> + * objcg - the obj_cgroup(actually memcg) this entry belongs to
> 
> 
> Again, not the place to document that objcg is an abstraction layer
> above a memcg.
> 
> I would do something like:
> objcg - the obj_cgroup that the compressed memory is charged to

Thanks for your review, I've just sent out the v2 patch.

> 
>>
>>   * lru - handle to the pool's lru used to evict pages.
>>   */
>>  struct zswap_entry {
>> --
>> 2.34.1
>>
>>

Yosry Ahmed Aug. 8, 2023, 6:36 a.m. UTC | #3

On Mon, Aug 7, 2023 at 11:26 PM xiujianfeng <[e-mail]> wrote:
>
> Hi Yosry,
>
> On 2023/8/8 13:59, Yosry Ahmed wrote:
> > Hi Xiu,
> >
> > On Mon, Aug 7, 2023 at 7:49 PM Xiu Jianfeng <[e-mail]> wrote:
> >>
> >> From: Xiu Jianfeng <[e-mail]>
> >>
> >> Since commit 0bb488498c98 ("mm: zswap: remove zswap_header"), the
> >> 'offset' has been replaced by swpentry, update the comment for it,
> >> and also add comment for 'objcg'.
> >>
> >> Signed-off-by: Xiu Jianfeng <[e-mail]>
> >> ---
> >>  mm/zswap.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/mm/zswap.c b/mm/zswap.c
> >> index 5b56d38e7339..d7863c139c82 100644
> >> --- a/mm/zswap.c
> >> +++ b/mm/zswap.c
> >> @@ -182,7 +182,7 @@ struct zswap_pool {
> >>   * page within zswap.
> >>   *
> >>   * rbnode - links the entry into red-black tree for the appropriate swap type
> >> - * offset - the swap offset for the entry.  Index into the red-black tree.
> >> + * swpentry - the swap entry for the entry, include 'type' and 'offset' fields
> >
> >
> > This is not the place to document that a swap entry has 'type' and
> > 'offset' fields.
> >
> > I would also avoid the repetitive 'entry'. Maybe something like:
> > swpentry - associated swap entry, the offset indexes into the red-black tree
> >
> >>
> >>   * refcount - the number of outstanding reference to the entry. This is needed
> >>   *            to protect against premature freeing of the entry by code
> >>   *            concurrent calls to load, invalidate, and writeback.  The lock
> >> @@ -195,6 +195,7 @@ struct zswap_pool {
> >>   * pool - the zswap_pool the entry's data is in
> >>   * handle - zpool allocation handle that stores the compressed page data
> >>   * value - value of the same-value filled pages which have same content
> >> + * objcg - the obj_cgroup(actually memcg) this entry belongs to
> >
> >
> > Again, not the place to document that objcg is an abstraction layer
> > above a memcg.
> >
> > I would do something like:
> > objcg - the obj_cgroup that the compressed memory is charged to
>
> Thanks for your review, I've just sent out the v2 patch.

I will take a look, thanks.

(a CC would have been convenient ;) )
>
> >
> >>
> >>   * lru - handle to the pool's lru used to evict pages.
> >>   */
> >>  struct zswap_entry {
> >> --
> >> 2.34.1
> >>
> >>

Patch

diff --git a/mm/zswap.c b/mm/zswap.c
index 5b56d38e7339..d7863c139c82 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -182,7 +182,7 @@  struct zswap_pool {
  * page within zswap.
  *
  * rbnode - links the entry into red-black tree for the appropriate swap type
- * offset - the swap offset for the entry.  Index into the red-black tree.
+ * swpentry - the swap entry for the entry, include 'type' and 'offset' fields
  * refcount - the number of outstanding reference to the entry. This is needed
  *            to protect against premature freeing of the entry by code
  *            concurrent calls to load, invalidate, and writeback.  The lock
@@ -195,6 +195,7 @@  struct zswap_pool {
  * pool - the zswap_pool the entry's data is in
  * handle - zpool allocation handle that stores the compressed page data
  * value - value of the same-value filled pages which have same content
+ * objcg - the obj_cgroup(actually memcg) this entry belongs to
  * lru - handle to the pool's lru used to evict pages.
  */
 struct zswap_entry {