lkml.org 
[lkml]   [2019]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [tip:x86/cache] x86/resctrl: Initialize a new resource group with default MBA values
From
Date
Hi Boris,

On 4/18/2019 15:28, Borislav Petkov wrote:
> On Thu, Apr 18, 2019 at 03:03:35PM +0800, Xiaochen Shen wrote:
>> In my opinion, this newline is unnecessary. Thank you.
>
> See commit message:
>
>> [ bp: Add newlines between code blocks for better readability. ]
>

I got this commit message and the code changes.
Really appreciated that you helped add several newlines between code
blocks. The newlines really make the readability of the code better.

> for_each_alloc_enabled_rdt_resource(r) {
> ...;
> ret = update_domains(r, rdtgrp->closid);
> if (ret < 0) {
> rdt_last_cmd_puts("Failed to initialize allocations\n");
> return ret;
> }
> +
> }

But is this newline an exception?
This newline is in the middle of two "}"s - the first "}" is the end of
if condition, and the second "}" is the end of
"for_each_alloc_enabled_rdt_resource" loop. I don't think the newline is
necessary.

> And I didn't add enough. That code is too crammed.
>
> For example, the new __init_one_rdt_domain() could use some newlines
> too, to separate code blocks for better readability. At least before
> each comment so that one can visually distinguish code groups
> better/faster.
>
> Here the whole function pasted with newlines added where I think they
> make sense. This way you have visual separation between the code blocks
> and not one big fat clump of code which one has to painstakingly pick
> apart.

Thank you very much for pointing this out and helping make the changes.

Should I submit a separate fixing patch for __init_one_rdt_domain()?
Thank you.

>
> static int __init_one_rdt_domain(struct rdt_domain *d, struct rdt_resource *r,
> u32 closid)
> {
> struct rdt_resource *r_cdp = NULL;
> struct rdt_domain *d_cdp = NULL;
> u32 used_b = 0, unused_b = 0;
> unsigned long tmp_cbm;
> enum rdtgrp_mode mode;
> u32 peer_ctl, *ctrl;
> int i;
>
> rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp);
> d->have_new_ctrl = false;
> d->new_ctrl = r->cache.shareable_bits;
> used_b = r->cache.shareable_bits;
> ctrl = d->ctrl_val;
>
> for (i = 0; i < closids_supported(); i++, ctrl++) {
> if (closid_allocated(i) && i != closid) {
> mode = rdtgroup_mode_by_closid(i);
> if (mode == RDT_MODE_PSEUDO_LOCKSETUP)
> break;
>
> /*
> * If CDP is active include peer domain's
> * usage to ensure there is no overlap
> * with an exclusive group.
> */
> if (d_cdp)
> peer_ctl = d_cdp->ctrl_val[i];
> else
> peer_ctl = 0;
>
> used_b |= *ctrl | peer_ctl;
> if (mode == RDT_MODE_SHAREABLE)
> d->new_ctrl |= *ctrl | peer_ctl;
> }
> }
>
> if (d->plr && d->plr->cbm > 0)
> used_b |= d->plr->cbm;
>
> unused_b = used_b ^ (BIT_MASK(r->cache.cbm_len) - 1);
> unused_b &= BIT_MASK(r->cache.cbm_len) - 1;
> d->new_ctrl |= unused_b;
>
> /*
> * Force the initial CBM to be valid, user can
> * modify the CBM based on system availability.
> */
> cbm_ensure_valid(&d->new_ctrl, r);
>
> /*
> * Assign the u32 CBM to an unsigned long to ensure that
> * bitmap_weight() does not access out-of-bound memory.
> */
> tmp_cbm = d->new_ctrl;
>
> if (bitmap_weight(&tmp_cbm, r->cache.cbm_len) < r->cache.min_cbm_bits) {
> rdt_last_cmd_printf("No space on %s:%d\n", r->name, d->id);
> return -ENOSPC;
> }
> d->have_new_ctrl = true;
>
> return 0;
> }
>

--
Best regards,
Xiaochen

\
 
 \ /
  Last update: 2019-04-18 10:21    [W:0.054 / U:1.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site