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

[Borislav Petkov]

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. ]

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.

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;
}

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.