Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 23 Mar 2023 16:46:07 -0500
From:   Steve Wahl <steve.wahl@hpe.com>
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     Steve Wahl <steve.wahl@hpe.com>,
        Dimitri Sivanich <dimitri.sivanich@hpe.com>,
        Russ Anderson <russ.anderson@hpe.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] x86/platform/uv: UV support for sub-NUMA clustering
Message-ID: <ZBzInwfGpzeoiWo6@swahl-home.5wahls.com>
References: <20230224225904.2618624-1-steve.wahl@hpe.com>
 <41a01b03-26f9-a9ae-d8f7-6b0eeb941cf5@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <41a01b03-26f9-a9ae-d8f7-6b0eeb941cf5@intel.com>
Precedence: bulk

Dave,

Thank you for taking the time to review. I will create another
version, doing my best to clarify and simplify.

--> Steve

On Thu, Mar 23, 2023 at 09:26:50AM -0700, Dave Hansen wrote:
> 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.

-- 
Steve Wahl, Hewlett Packard Enterprise