linux-kernel - Re: [tip:x86/cache] x86/resctrl: Initialize a new resource group with default MBA values

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <27c69929-58ee-0a96-0953-e7cfa8976e78@intel.com>
Date:   Thu, 18 Apr 2019 16:20:57 +0800
From:   Xiaochen Shen <xiaochen.shen@...el.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     tony.luck@...el.com, x86@...nel.org, mingo@...hat.com, bp@...e.de,
        reinette.chatre@...el.com, hpa@...or.com, tglx@...utronix.de,
        mingo@...nel.org, linux-kernel@...r.kernel.org,
        fenghua.yu@...el.com, linux-tip-commits@...r.kernel.org,
        Xiaochen Shen <xiaochen.shen@...el.com>
Subject: Re: [tip:x86/cache] x86/resctrl: Initialize a new resource group with
 default MBA values

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