linux-kernel - Re: [PATCH v4] x86/platform/uv: UV support for sub-NUMA clustering

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <41a01b03-26f9-a9ae-d8f7-6b0eeb941cf5@intel.com>
Date:   Thu, 23 Mar 2023 09:26:50 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Steve Wahl <steve.wahl@....com>,
        Dimitri Sivanich <dimitri.sivanich@....com>,
        Russ Anderson <russ.anderson@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] x86/platform/uv: UV support for sub-NUMA clustering

On 2/24/23 14:59, Steve Wahl wrote:
> +			if (np) {
> +				s = ((i * 64) + __ffs(np)) & s_mask;
> +				if (sock_min > s)
> +					sock_min = s;
> +				s = ((i * 64) + __fls(np)) & s_mask;
> +				if (sock_max < s)
> +					sock_max = s;
> +			}
>  		}
>  	}
>  	if (UVH_NODE_PRESENT_0) {
>  		np = uv_read_local_mmr(UVH_NODE_PRESENT_0);
>  		pr_info("UV: NODE_PRESENT_0 = 0x%016lx\n", np);
> -		uv_pb += hweight64(np);
> +		if (np) {
> +			s = __ffs(np) & s_mask;
> +			if (sock_min > s)
> +				sock_min = s;
> +			s = __fls(np) & s_mask;
> +			if (sock_max < s)
> +				sock_max = s;
> +		}
>  	}
>  	if (UVH_NODE_PRESENT_1) {
>  		np = uv_read_local_mmr(UVH_NODE_PRESENT_1);
>  		pr_info("UV: NODE_PRESENT_1 = 0x%016lx\n", np);
> -		uv_pb += hweight64(np);
> +		if (np) {
> +			s = (64 + __ffs(np)) & s_mask;
> +			if (sock_min > s)
> +				sock_min = s;
> +			s = (64 + __fls(np)) & s_mask;
> +			if (sock_max < s)
> +				sock_max = s;
> +		}
> +	}

I guess this patch is modifying code that very few people care about.
But, this is kinda a mess.  This patch does a *TON* of different things
and makes almost no effort to refactor the code before diving into the
changes.

I quoted the above text because whatever that code is doing, it's
gloriously uncommented.  That kind of thing demands a helper even if
it's just used once so that a read can have _some_ idea what it's doing
logically.

Could you please take another pass at this?  I think it demands to be
broken up into at _least_ 5-10 individual patches.

For instance, you could introduce and use uv_pnode_to_socket() in one patch.

Or this:

> -		nasid_mask = UVH_RH_GAM_MMIOH_OVERLAY_CONFIG0_BASE_MASK;
> +		nasid_mask =
> +			is_uv(UV4A) ? UV4AH_RH_GAM_MMIOH_REDIRECT_CONFIG0_NASID_MASK :
> +			is_uv(UV4)  ? UV4H_RH_GAM_MMIOH_REDIRECT_CONFIG0_NASID_MASK :
> +			UV3H_RH_GAM_MMIOH_REDIRECT_CONFIG0_NASID_MASK;
>  		n = UVH_RH_GAM_MMIOH_REDIRECT_CONFIG0_DEPTH;
>  		min_nasid = min_pnode * 2;
>  		max_nasid = max_pnode * 2;
> @@ -1046,7 +1050,10 @@ static void __init calc_mmioh_map(enum mmioh_arch index,
>  		break;
>  	case UVX_MMIOH1:
>  		mmr = UVH_RH_GAM_MMIOH_REDIRECT_CONFIG1;
> -		nasid_mask = UVH_RH_GAM_MMIOH_OVERLAY_CONFIG1_BASE_MASK;
> +		nasid_mask =
> +			is_uv(UV4A) ? UV4AH_RH_GAM_MMIOH_REDIRECT_CONFIG0_NASID_MASK :
> +			is_uv(UV4)  ? UV4H_RH_GAM_MMIOH_REDIRECT_CONFIG1_NASID_MASK :
> +			UV3H_RH_GAM_MMIOH_REDIRECT_CONFIG1_NASID_MASK;

That could use a helper to reduce the duplication and add clarity and be
done in a separate patch.