The references in the is_uvX_hub() function uses the hub_info pointer
which will be NULL when the system is hubless. This change avoids
that NULL dereference. It is also an optimization in performance.
Signed-off-by: Mike Travis <[email protected]>
Reviewed-by: Steve Wahl <[email protected]>
Reviewed-by: Dimitri Sivanich <[email protected]>
---
arch/x86/include/asm/uv/.uv_hub.h.swp |binary
arch/x86/include/asm/uv/uv_hub.h | 60 ++++++++++++---------------------------
1 file changed, 19 insertions(+), 41 deletions(-)
Binary files linux.orig/arch/x86/include/asm/uv/.uv_hub.h.swp and linux/arch/x86/include/asm/uv/.uv_hub.h.swp differ
--- linux.orig/arch/x86/include/asm/uv/uv_hub.h
+++ linux/arch/x86/include/asm/uv/uv_hub.h
@@ -19,6 +19,7 @@
#include <linux/topology.h>
#include <asm/types.h>
#include <asm/percpu.h>
+#include <asm/uv/uv.h>
#include <asm/uv/uv_mmrs.h>
#include <asm/uv/bios.h>
#include <asm/irq_vectors.h>
@@ -243,83 +244,60 @@ static inline int uv_hub_info_check(int
#define UV4_HUB_REVISION_BASE 7
#define UV4A_HUB_REVISION_BASE 8 /* UV4 (fixed) rev 2 */
-#ifdef UV1_HUB_IS_SUPPORTED
static inline int is_uv1_hub(void)
{
- return uv_hub_info->hub_revision < UV2_HUB_REVISION_BASE;
-}
+#ifdef UV1_HUB_IS_SUPPORTED
+ return is_uv_hubbed(uv(1));
#else
-static inline int is_uv1_hub(void)
-{
return 0;
-}
#endif
+}
-#ifdef UV2_HUB_IS_SUPPORTED
static inline int is_uv2_hub(void)
{
- return ((uv_hub_info->hub_revision >= UV2_HUB_REVISION_BASE) &&
- (uv_hub_info->hub_revision < UV3_HUB_REVISION_BASE));
-}
+#ifdef UV2_HUB_IS_SUPPORTED
+ return is_uv_hubbed(uv(2));
#else
-static inline int is_uv2_hub(void)
-{
return 0;
-}
#endif
+}
-#ifdef UV3_HUB_IS_SUPPORTED
static inline int is_uv3_hub(void)
{
- return ((uv_hub_info->hub_revision >= UV3_HUB_REVISION_BASE) &&
- (uv_hub_info->hub_revision < UV4_HUB_REVISION_BASE));
-}
+#ifdef UV3_HUB_IS_SUPPORTED
+ return is_uv_hubbed(uv(3));
#else
-static inline int is_uv3_hub(void)
-{
return 0;
-}
#endif
+}
/* First test "is UV4A", then "is UV4" */
-#ifdef UV4A_HUB_IS_SUPPORTED
-static inline int is_uv4a_hub(void)
-{
- return (uv_hub_info->hub_revision >= UV4A_HUB_REVISION_BASE);
-}
-#else
static inline int is_uv4a_hub(void)
{
+#ifdef UV4A_HUB_IS_SUPPORTED
+ if (is_uv_hubbed(uv(4)))
+ return (uv_hub_info->hub_revision == UV4A_HUB_REVISION_BASE);
+#endif
return 0;
}
-#endif
-#ifdef UV4_HUB_IS_SUPPORTED
static inline int is_uv4_hub(void)
{
- return uv_hub_info->hub_revision >= UV4_HUB_REVISION_BASE;
-}
+#ifdef UV4_HUB_IS_SUPPORTED
+ return is_uv_hubbed(uv(4));
#else
-static inline int is_uv4_hub(void)
-{
return 0;
-}
#endif
+}
static inline int is_uvx_hub(void)
{
- if (uv_hub_info->hub_revision >= UV2_HUB_REVISION_BASE)
- return uv_hub_info->hub_revision;
-
- return 0;
+ return (is_uv_hubbed(-2) >= uv(2));
}
static inline int is_uv_hub(void)
{
-#ifdef UV1_HUB_IS_SUPPORTED
- return uv_hub_info->hub_revision;
-#endif
- return is_uvx_hub();
+ return is_uv1_hub() || is_uvx_hub();
}
union uvh_apicid {
--
On Mon, Sep 02, 2019 at 07:18:23PM -0500, Mike Travis wrote:
> +#ifdef UV1_HUB_IS_SUPPORTED
All these ifdefs are dead code, please just remove them.
Also it seems like at least the various mmr macros just check
for a specific version, I think you are much better off just
using a switch statement for the possible revisions there.
> + return (uv_hub_info->hub_revision == UV4A_HUB_REVISION_BASE);
And none of these braces are required.
On 9/3/2019 9:19 AM, Christoph Hellwig wrote:
> On Mon, Sep 02, 2019 at 07:18:23PM -0500, Mike Travis wrote:
>> +#ifdef UV1_HUB_IS_SUPPORTED
>
> All these ifdefs are dead code, please just remove them.
Those ifdefs are not dead code and are being actively used. Plus UV1
support is dead and I think the last running system died about a year
ago and no support or parts are available. So undef'ing these macros
will simplify and reduce the size of the object code.
> Also it seems like at least the various mmr macros just check
> for a specific version, I think you are much better off just
> using a switch statement for the possible revisions there.
>
>> + return (uv_hub_info->hub_revision == UV4A_HUB_REVISION_BASE);
The problem is those revision bases can change if a UV HUB revision
changes. That is why there are ranges and why I'm converting them to
"uv_type". Some UV kernel source code still needs to know the exact HUB
revision, like (again) hwperf.
>
> And none of these braces are required.
>
Sure, I can take those out now, but usually I then get bit by
checkpatches which then says "parenthesis's are required".
On 9/3/2019 11:58 AM, Mike Travis wrote:
>
>
> On 9/3/2019 9:19 AM, Christoph Hellwig wrote:
>> On Mon, Sep 02, 2019 at 07:18:23PM -0500, Mike Travis wrote:
>>> +#ifdef??? UV1_HUB_IS_SUPPORTED
>>
>> All these ifdefs are dead code, please just remove them.
>
> Those ifdefs are not dead code and are being actively used.? Plus UV1
> support is dead and I think the last running system died about a year
> ago and no support or parts are available.? So undef'ing these macros
> will simplify and reduce the size of the object code.
I forgot to add that if we do undef one of those "is supported" the code
will eventually be removed, thus simplifying the source even more. So
including the ifdef's in the source make that code easier to find.
>
>> Also it seems like at least the various mmr macros just check
>> for a specific version, I think you are much better off just
>> using a switch statement for the possible revisions there.
>>
>>> +??????? return (uv_hub_info->hub_revision == UV4A_HUB_REVISION_BASE);
>
> The problem is those revision bases can change if a UV HUB revision
> changes.? That is why there are ranges and why I'm converting them to
> "uv_type".? Some UV kernel source code still needs to know the exact HUB
> revision, like (again) hwperf.
>
>>
>> And none of these braces are required.
>>
>
> Sure, I can take those out now, but usually I then get bit by
> checkpatches which then says "parenthesis's are required".
On Tue, Sep 03, 2019 at 11:58:49AM -0700, Mike Travis wrote:
> Those ifdefs are not dead code and are being actively used. Plus UV1
> support is dead and I think the last running system died about a year ago
> and no support or parts are available. So undef'ing these macros will
> simplify and reduce the size of the object code.
I'm not complaining about removing some ifdefs, that is always good.
I complain about keeping the others that are dead. And if Hub 1 is
dead please drop all the checks and support code for it.
A patch against current mainline to show what I mean is below.
---
From e84506399fa9436d47b33491d3e38e9dc3c718c7 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Tue, 3 Sep 2019 18:05:37 +0200
Subject: x86/uv: Remove the dead UV?_HUB_IS_SUPPORTED defines
These are always set, so remove them and the dead code for the case
where they are not defined.
Signed-off-by: Christoph Hellwig <[email protected]>
---
arch/x86/include/asm/uv/uv_hub.h | 38 -------------------------------
arch/x86/include/asm/uv/uv_mmrs.h | 7 ------
2 files changed, 45 deletions(-)
diff --git a/arch/x86/include/asm/uv/uv_hub.h b/arch/x86/include/asm/uv/uv_hub.h
index 6eed0b379412..f71eb659f0de 100644
--- a/arch/x86/include/asm/uv/uv_hub.h
+++ b/arch/x86/include/asm/uv/uv_hub.h
@@ -229,68 +229,33 @@ static inline struct uv_hub_info_s *uv_cpu_hub_info(int cpu)
#define UV4_HUB_REVISION_BASE 7
#define UV4A_HUB_REVISION_BASE 8 /* UV4 (fixed) rev 2 */
-#ifdef UV1_HUB_IS_SUPPORTED
static inline int is_uv1_hub(void)
{
return uv_hub_info->hub_revision < UV2_HUB_REVISION_BASE;
}
-#else
-static inline int is_uv1_hub(void)
-{
- return 0;
-}
-#endif
-#ifdef UV2_HUB_IS_SUPPORTED
static inline int is_uv2_hub(void)
{
return ((uv_hub_info->hub_revision >= UV2_HUB_REVISION_BASE) &&
(uv_hub_info->hub_revision < UV3_HUB_REVISION_BASE));
}
-#else
-static inline int is_uv2_hub(void)
-{
- return 0;
-}
-#endif
-#ifdef UV3_HUB_IS_SUPPORTED
static inline int is_uv3_hub(void)
{
return ((uv_hub_info->hub_revision >= UV3_HUB_REVISION_BASE) &&
(uv_hub_info->hub_revision < UV4_HUB_REVISION_BASE));
}
-#else
-static inline int is_uv3_hub(void)
-{
- return 0;
-}
-#endif
/* First test "is UV4A", then "is UV4" */
-#ifdef UV4A_HUB_IS_SUPPORTED
static inline int is_uv4a_hub(void)
{
return (uv_hub_info->hub_revision >= UV4A_HUB_REVISION_BASE);
}
-#else
-static inline int is_uv4a_hub(void)
-{
- return 0;
-}
-#endif
-#ifdef UV4_HUB_IS_SUPPORTED
static inline int is_uv4_hub(void)
{
return uv_hub_info->hub_revision >= UV4_HUB_REVISION_BASE;
}
-#else
-static inline int is_uv4_hub(void)
-{
- return 0;
-}
-#endif
static inline int is_uvx_hub(void)
{
@@ -302,10 +267,7 @@ static inline int is_uvx_hub(void)
static inline int is_uv_hub(void)
{
-#ifdef UV1_HUB_IS_SUPPORTED
return uv_hub_info->hub_revision;
-#endif
- return is_uvx_hub();
}
union uvh_apicid {
diff --git a/arch/x86/include/asm/uv/uv_mmrs.h b/arch/x86/include/asm/uv/uv_mmrs.h
index 62c79e26a59a..9ee5ed6e8b34 100644
--- a/arch/x86/include/asm/uv/uv_mmrs.h
+++ b/arch/x86/include/asm/uv/uv_mmrs.h
@@ -99,13 +99,6 @@
#define UV3_HUB_PART_NUMBER_X 0x4321
#define UV4_HUB_PART_NUMBER 0x99a1
-/* Compat: Indicate which UV Hubs are supported. */
-#define UV1_HUB_IS_SUPPORTED 1
-#define UV2_HUB_IS_SUPPORTED 1
-#define UV3_HUB_IS_SUPPORTED 1
-#define UV4_HUB_IS_SUPPORTED 1
-#define UV4A_HUB_IS_SUPPORTED 1
-
/* Error function to catch undefined references */
extern unsigned long uv_undefined(char *str);
--
2.20.1
Hi Christoph,
I can do these and leave some comments as markers to find the code to
remove later. The real problem is uv_mmrs.h is a generated file and the
scripts cannot be easily redone. Basically they copy in the verilog
definition files from the UV HUB design files and create kernel
compatible files that also then generate run-time checking of hardware
specifics like MMR addresses and field definitions. The change to
remove those "is supported defines" takes a higher level process that
cannot be done within this short release cycle. We are racing to
include this support in the next linux release for pending new hardware
introductions.
Also that script is in a state of flux for the next UV arch and
processor updates so changing anything at this moment is highly
disruptive. Is there a specific reason why this UV only code that does
not touch anything else in the kernel and only affects our productivity
and system performance appears to be such a problem with you? And why
now, at this moment, and after 10 years does it suddenly seem so
important? I'm all for clean up but not disruption.
Bottom line, I agree to all these changes and I promise to tend to them
in the next release cycle. But please let us get through this cycle
without this much chaos?
Thanks,
Mike
On 9/3/2019 11:52 PM, Christoph Hellwig wrote:
> On Tue, Sep 03, 2019 at 11:58:49AM -0700, Mike Travis wrote:
>> Those ifdefs are not dead code and are being actively used. Plus UV1
>> support is dead and I think the last running system died about a year ago
>> and no support or parts are available. So undef'ing these macros will
>> simplify and reduce the size of the object code.
>
> I'm not complaining about removing some ifdefs, that is always good.
> I complain about keeping the others that are dead. And if Hub 1 is
> dead please drop all the checks and support code for it.
>
> A patch against current mainline to show what I mean is below.
>
> ---
> From e84506399fa9436d47b33491d3e38e9dc3c718c7 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <[email protected]>
> Date: Tue, 3 Sep 2019 18:05:37 +0200
> Subject: x86/uv: Remove the dead UV?_HUB_IS_SUPPORTED defines
>
> These are always set, so remove them and the dead code for the case
> where they are not defined.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> arch/x86/include/asm/uv/uv_hub.h | 38 -------------------------------
> arch/x86/include/asm/uv/uv_mmrs.h | 7 ------
> 2 files changed, 45 deletions(-)
>
> diff --git a/arch/x86/include/asm/uv/uv_hub.h b/arch/x86/include/asm/uv/uv_hub.h
> index 6eed0b379412..f71eb659f0de 100644
> --- a/arch/x86/include/asm/uv/uv_hub.h
> +++ b/arch/x86/include/asm/uv/uv_hub.h
> @@ -229,68 +229,33 @@ static inline struct uv_hub_info_s *uv_cpu_hub_info(int cpu)
> #define UV4_HUB_REVISION_BASE 7
> #define UV4A_HUB_REVISION_BASE 8 /* UV4 (fixed) rev 2 */
>
> -#ifdef UV1_HUB_IS_SUPPORTED
> static inline int is_uv1_hub(void)
> {
> return uv_hub_info->hub_revision < UV2_HUB_REVISION_BASE;
> }
> -#else
> -static inline int is_uv1_hub(void)
> -{
> - return 0;
> -}
> -#endif
>
> -#ifdef UV2_HUB_IS_SUPPORTED
> static inline int is_uv2_hub(void)
> {
> return ((uv_hub_info->hub_revision >= UV2_HUB_REVISION_BASE) &&
> (uv_hub_info->hub_revision < UV3_HUB_REVISION_BASE));
> }
> -#else
> -static inline int is_uv2_hub(void)
> -{
> - return 0;
> -}
> -#endif
>
> -#ifdef UV3_HUB_IS_SUPPORTED
> static inline int is_uv3_hub(void)
> {
> return ((uv_hub_info->hub_revision >= UV3_HUB_REVISION_BASE) &&
> (uv_hub_info->hub_revision < UV4_HUB_REVISION_BASE));
> }
> -#else
> -static inline int is_uv3_hub(void)
> -{
> - return 0;
> -}
> -#endif
>
> /* First test "is UV4A", then "is UV4" */
> -#ifdef UV4A_HUB_IS_SUPPORTED
> static inline int is_uv4a_hub(void)
> {
> return (uv_hub_info->hub_revision >= UV4A_HUB_REVISION_BASE);
> }
> -#else
> -static inline int is_uv4a_hub(void)
> -{
> - return 0;
> -}
> -#endif
>
> -#ifdef UV4_HUB_IS_SUPPORTED
> static inline int is_uv4_hub(void)
> {
> return uv_hub_info->hub_revision >= UV4_HUB_REVISION_BASE;
> }
> -#else
> -static inline int is_uv4_hub(void)
> -{
> - return 0;
> -}
> -#endif
>
> static inline int is_uvx_hub(void)
> {
> @@ -302,10 +267,7 @@ static inline int is_uvx_hub(void)
>
> static inline int is_uv_hub(void)
> {
> -#ifdef UV1_HUB_IS_SUPPORTED
> return uv_hub_info->hub_revision;
> -#endif
> - return is_uvx_hub();
> }
>
> union uvh_apicid {
> diff --git a/arch/x86/include/asm/uv/uv_mmrs.h b/arch/x86/include/asm/uv/uv_mmrs.h
> index 62c79e26a59a..9ee5ed6e8b34 100644
> --- a/arch/x86/include/asm/uv/uv_mmrs.h
> +++ b/arch/x86/include/asm/uv/uv_mmrs.h
> @@ -99,13 +99,6 @@
> #define UV3_HUB_PART_NUMBER_X 0x4321
> #define UV4_HUB_PART_NUMBER 0x99a1
>
> -/* Compat: Indicate which UV Hubs are supported. */
> -#define UV1_HUB_IS_SUPPORTED 1
> -#define UV2_HUB_IS_SUPPORTED 1
> -#define UV3_HUB_IS_SUPPORTED 1
> -#define UV4_HUB_IS_SUPPORTED 1
> -#define UV4A_HUB_IS_SUPPORTED 1
> -
> /* Error function to catch undefined references */
> extern unsigned long uv_undefined(char *str);
>
>