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

[Xiaochen Shen]

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