[PATCH][RFC] parisc: Change L1_CACHE_BYTES to 16

[PATCH][RFC] parisc: Change L1_CACHE_BYTES to 16

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

The attached change reduces L1_CACHE_BYTES from 32 on PA1.1 and 64 on PA2.0 to 16.

This is based on examination of the L1 cache design for the PA-8700 processor where it can
be seen the processor loads two double words per cycle.  This line length is consistent with the
original alignment requirement for the ldcw instruction.

Thus, we need to distinguish between the L1 and L2 (SMP) cache line lengths.

The attached change bumps SMP_CACHE_BYTES to 128 bytes as this is the line length used
on PA-8800 and PA-8900 processors.  This increases the overall kernel size somewhat but seems
logically correct.

I left ARCH_DMA_MINALIGN at L1_CACHE_BYTES as that's the define most architectures use.

There are a few uses of L1_CACHE_BYTES in drivers/parisc.  This is the code that's most likely
to break from the change.

I have lightly tested the change on c8000 running 4.2.1+.

Signed-off-by: John David Anglin <dave.anglin@bell.net>


[-- Attachment #2: cache.h.d.txt --]
[-- Type: text/plain, Size: 1228 bytes --]

diff --git a/arch/parisc/include/asm/cache.h b/arch/parisc/include/asm/cache.h
index 47f11c7..e92a355 100644
--- a/arch/parisc/include/asm/cache.h
+++ b/arch/parisc/include/asm/cache.h
@@ -7,24 +7,23 @@
 
 
 /*
- * PA 2.0 processors have 64-byte cachelines; PA 1.1 processors have
- * 32-byte cachelines.  The default configuration is not for SMP anyway,
+ * PA 2.0 processors have 64 and 128-byte L2 cachelines; PA 1.1 processors
+ * have 32-byte cachelines.  The default configuration is not for SMP anyway,
  * so if you're building for SMP, you should select the appropriate
  * processor type.  There is a potential livelock danger when running
  * a machine with this value set too small, but it's more probable you'll
  * just ruin performance.
  */
-#ifdef CONFIG_PA20
-#define L1_CACHE_BYTES 64
-#define L1_CACHE_SHIFT 6
-#else
-#define L1_CACHE_BYTES 32
-#define L1_CACHE_SHIFT 5
-#endif
+#define L1_CACHE_BYTES 16
+#define L1_CACHE_SHIFT 4
 
 #ifndef __ASSEMBLY__
 
-#define SMP_CACHE_BYTES L1_CACHE_BYTES
+#ifdef CONFIG_PA20
+#define SMP_CACHE_BYTES (8 * L1_CACHE_BYTES)
+#else
+#define SMP_CACHE_BYTES (2 * L1_CACHE_BYTES)
+#endif
 
 #define ARCH_DMA_MINALIGN	L1_CACHE_BYTES
 

[-- Attachment #3: Type: text/plain, Size: 44 bytes --]



--
John David Anglin	dave.anglin@bell.net

[PATCH v2][RFC] parisc: Change L1_CACHE_BYTES to 16

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

On 2015-09-26, at 11:38 AM, John David Anglin wrote:

> The attached change reduces L1_CACHE_BYTES from 32 on PA1.1 and 64 on PA2.0 to 16.
> 
> This is based on examination of the L1 cache design for the PA-8700 processor where it can
> be seen the processor loads two double words per cycle.  This line length is consistent with the
> original alignment requirement for the ldcw instruction.
> 
> Thus, we need to distinguish between the L1 and L2 (SMP) cache line lengths.
> 
> The attached change bumps SMP_CACHE_BYTES to 128 bytes as this is the line length used
> on PA-8800 and PA-8900 processors.  This increases the overall kernel size somewhat but seems
> logically correct.


Slight tweak to previous.  This version only bumps SMP_CACHE_BYTES when CONFIG_SMP
is defined.

Signed-off-by: John David Anglin <dave.anglin@bell.net>



[-- Attachment #2: cache.h.d.txt --]
[-- Type: text/plain, Size: 1265 bytes --]

diff --git a/arch/parisc/include/asm/cache.h b/arch/parisc/include/asm/cache.h
index 47f11c7..cf11dd4 100644
--- a/arch/parisc/include/asm/cache.h
+++ b/arch/parisc/include/asm/cache.h
@@ -7,24 +7,27 @@
 
 
 /*
- * PA 2.0 processors have 64-byte cachelines; PA 1.1 processors have
- * 32-byte cachelines.  The default configuration is not for SMP anyway,
+ * PA 2.0 processors have 64 and 128-byte L2 cachelines; PA 1.1 processors
+ * have 32-byte cachelines.  The default configuration is not for SMP anyway,
  * so if you're building for SMP, you should select the appropriate
  * processor type.  There is a potential livelock danger when running
  * a machine with this value set too small, but it's more probable you'll
  * just ruin performance.
  */
-#ifdef CONFIG_PA20
-#define L1_CACHE_BYTES 64
-#define L1_CACHE_SHIFT 6
-#else
-#define L1_CACHE_BYTES 32
-#define L1_CACHE_SHIFT 5
-#endif
+#define L1_CACHE_BYTES 16
+#define L1_CACHE_SHIFT 4
 
 #ifndef __ASSEMBLY__
 
+#ifdef CONFIG_SMP
+#ifdef CONFIG_PA20
+#define SMP_CACHE_BYTES (8 * L1_CACHE_BYTES)
+#else
+#define SMP_CACHE_BYTES (2 * L1_CACHE_BYTES)
+#endif
+#else
 #define SMP_CACHE_BYTES L1_CACHE_BYTES
+#endif
 
 #define ARCH_DMA_MINALIGN	L1_CACHE_BYTES
 

[-- Attachment #3: Type: text/plain, Size: 44 bytes --]



--
John David Anglin	dave.anglin@bell.net

Re: [PATCH v2][RFC] parisc: Change L1_CACHE_BYTES to 16

[James Bottomley]

On Sun, 2015-09-27 at 12:07 -0400, John David Anglin wrote:
> On 2015-09-26, at 11:38 AM, John David Anglin wrote:
> 
> > The attached change reduces L1_CACHE_BYTES from 32 on PA1.1 and 64 on PA2.0 to 16.
> > 
> > This is based on examination of the L1 cache design for the PA-8700 processor where it can
> > be seen the processor loads two double words per cycle.  This line length is consistent with the
> > original alignment requirement for the ldcw instruction.
> > 
> > Thus, we need to distinguish between the L1 and L2 (SMP) cache line lengths.
> > 
> > The attached change bumps SMP_CACHE_BYTES to 128 bytes as this is the line length used
> > on PA-8800 and PA-8900 processors.  This increases the overall kernel size somewhat but seems
> > logically correct.
> 
> 
> Slight tweak to previous.  This version only bumps SMP_CACHE_BYTES when CONFIG_SMP
> is defined.

What makes you think we need SMP_CACHE_BYTES to be different from
L1_CACHE_BYTES?  No other architecture does this.  The theory that gives
us two defines was that some SMP systems would arbitrate for memory at
geater than cache line offsets but, in practise, none does because
that's the level at which the cross CPU memory ownership model works
anyway.

James

Re: [PATCH v2][RFC] parisc: Change L1_CACHE_BYTES to 16

[John David Anglin]

On 2015-09-27, at 12:17 PM, James Bottomley wrote:

> What makes you think we need SMP_CACHE_BYTES to be different from
> L1_CACHE_BYTES?  No other architecture does this.  The theory that gives
> us two defines was that some SMP systems would arbitrate for memory at
> geater than cache line offsets but, in practise, none does because
> that's the level at which the cross CPU memory ownership model works
> anyway.

I was just keeping the values we had before.  Changing to the new L1_CACHE_BYTES
value probably needs testing.

Dave
--
John David Anglin	dave.anglin@bell.net

[PATCH v3] parisc: Change L1_CACHE_BYTES to 16

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 211 bytes --]

The latest version changes L1_CACHE_BYTES to 16 bytes and doesn't adjust SMP_CACHE_BYTES as
per James suggestion

Tested for 16 days on rp3440.

Signed-off-by: John David Anglin <dave.anglin@bell.net>



[-- Attachment #2: cache.h.d.1.txt --]
[-- Type: text/plain, Size: 1035 bytes --]

diff --git a/arch/parisc/include/asm/cache.h b/arch/parisc/include/asm/cache.h
index 47f11c7..3d0e17b 100644
--- a/arch/parisc/include/asm/cache.h
+++ b/arch/parisc/include/asm/cache.h
@@ -7,20 +7,12 @@
 
 
 /*
- * PA 2.0 processors have 64-byte cachelines; PA 1.1 processors have
- * 32-byte cachelines.  The default configuration is not for SMP anyway,
- * so if you're building for SMP, you should select the appropriate
- * processor type.  There is a potential livelock danger when running
- * a machine with this value set too small, but it's more probable you'll
- * just ruin performance.
+ * PA 2.0 processors have 64 and 128-byte L2 cachelines; PA 1.1 processors
+ * have 32-byte cachelines.  The L1 length appears to be 16 bytes but this
+ * is not clearly documented.
  */
-#ifdef CONFIG_PA20
-#define L1_CACHE_BYTES 64
-#define L1_CACHE_SHIFT 6
-#else
-#define L1_CACHE_BYTES 32
-#define L1_CACHE_SHIFT 5
-#endif
+#define L1_CACHE_BYTES 16
+#define L1_CACHE_SHIFT 4
 
 #ifndef __ASSEMBLY__
 

[-- Attachment #3: Type: text/plain, Size: 44 bytes --]



--
John David Anglin	dave.anglin@bell.net

Aw: [PATCH v3] parisc: Change L1_CACHE_BYTES to 16

[Helge Deller]

> Betreff: [PATCH v3] parisc: Change L1_CACHE_BYTES to 16
> The latest version changes L1_CACHE_BYTES to 16 bytes and doesn't adj=
ust SMP_CACHE_BYTES as
> per James suggestion

This patch sadly breaks build on Linux kernel git head:

In file included from /home/cvs/LINUX/git-kernel/linux-2.6/net/core/dev=
=2Ec:92:0:
/net/core/dev.c: In function =E2=80=98expand_xps_map=E2=80=99:
/include/linux/netdevice.h:721:27: warning: overflow in implicit consta=
nt conversion [-Woverflow]
 #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map)) \
/net/core/dev.c:1972:18: note: in expansion of macro =E2=80=98XPS_MIN_M=
AP_ALLOC=E2=80=99
  int alloc_len =3D XPS_MIN_MAP_ALLOC;
                  ^
Helge
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
 in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Aw: [PATCH v3] parisc: Change L1_CACHE_BYTES to 16

[Helge Deller]

> > Betreff: [PATCH v3] parisc: Change L1_CACHE_BYTES to 16
> > The latest version changes L1_CACHE_BYTES to 16 bytes and doesn't a=
djust SMP_CACHE_BYTES as
> > per James suggestion
>=20
> This patch sadly breaks build on Linux kernel git head:
>=20
> In file included from /home/cvs/LINUX/git-kernel/linux-2.6/net/core/d=
ev.c:92:0:
> /net/core/dev.c: In function =E2=80=98expand_xps_map=E2=80=99:
> /include/linux/netdevice.h:721:27: warning: overflow in implicit cons=
tant conversion [-Woverflow]
>  #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))=
 \
> /net/core/dev.c:1972:18: note: in expansion of macro =E2=80=98XPS_MIN=
_MAP_ALLOC=E2=80=99
>   int alloc_len =3D XPS_MIN_MAP_ALLOC;

This is one possibility to fix it:

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2d15e38..21cf683 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -718,7 +718,7 @@ struct xps_map {
        u16 queues[0];
 };
 #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(=
u16)))
-#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))  =
 \
+#define XPS_MIN_MAP_ALLOC ( (L1_CACHE_BYTES > sizeof(struct xps_map) ?=
 L1_CACHE_BYTES : 2 * L1_CACHE_BYTES) - sizeof(struct xps_map)   \
     / sizeof(u16))
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
 in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Aw: [PATCH v3] parisc: Change L1_CACHE_BYTES to 16

[James Bottomley]

On Thu, 2015-10-22 at 13:53 +0200, Helge Deller wrote:
> > > Betreff: [PATCH v3] parisc: Change L1_CACHE_BYTES to 16
> > > The latest version changes L1_CACHE_BYTES to 16 bytes and doesn't=
 adjust SMP_CACHE_BYTES as
> > > per James suggestion
> >=20
> > This patch sadly breaks build on Linux kernel git head:
> >=20
> > In file included from /home/cvs/LINUX/git-kernel/linux-2.6/net/core=
/dev.c:92:0:
> > /net/core/dev.c: In function =E2=80=98expand_xps_map=E2=80=99:
> > /include/linux/netdevice.h:721:27: warning: overflow in implicit co=
nstant conversion [-Woverflow]
> >  #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map=
)) \
> > /net/core/dev.c:1972:18: note: in expansion of macro =E2=80=98XPS_M=
IN_MAP_ALLOC=E2=80=99
> >   int alloc_len =3D XPS_MIN_MAP_ALLOC;
>=20
> This is one possibility to fix it:
>=20
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 2d15e38..21cf683 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -718,7 +718,7 @@ struct xps_map {
>         u16 queues[0];
>  };
>  #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeo=
f(u16)))
> -#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))=
   \
> +#define XPS_MIN_MAP_ALLOC ( (L1_CACHE_BYTES > sizeof(struct xps_map)=
 ? L1_CACHE_BYTES : 2 * L1_CACHE_BYTES) - sizeof(struct xps_map)   \
>      / sizeof(u16))

But it indicates the whole kernel has the architectural assumption that
L1_CACHE_BYTES >=3D 32, so I don't think we should be trying to change
that.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
 in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Aw: [PATCH v3] parisc: Change L1_CACHE_BYTES to 16

[John David Anglin]

On 2015-10-22 10:35 AM, James Bottomley wrote:
> On Thu, 2015-10-22 at 13:53 +0200, Helge Deller wrote:
>>>> Betreff: [PATCH v3] parisc: Change L1_CACHE_BYTES to 16
>>>> The latest version changes L1_CACHE_BYTES to 16 bytes and doesn't =
adjust SMP_CACHE_BYTES as
>>>> per James suggestion
>>> This patch sadly breaks build on Linux kernel git head:
>>>
>>> In file included from /home/cvs/LINUX/git-kernel/linux-2.6/net/core=
/dev.c:92:0:
>>> /net/core/dev.c: In function =E2=80=98expand_xps_map=E2=80=99:
>>> /include/linux/netdevice.h:721:27: warning: overflow in implicit co=
nstant conversion [-Woverflow]
>>>   #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_ma=
p)) \
>>> /net/core/dev.c:1972:18: note: in expansion of macro =E2=80=98XPS_M=
IN_MAP_ALLOC=E2=80=99
>>>    int alloc_len =3D XPS_MIN_MAP_ALLOC;
>> This is one possibility to fix it:
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 2d15e38..21cf683 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -718,7 +718,7 @@ struct xps_map {
>>          u16 queues[0];
>>   };
>>   #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * siz=
eof(u16)))
>> -#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map)=
)   \
>> +#define XPS_MIN_MAP_ALLOC ( (L1_CACHE_BYTES > sizeof(struct xps_map=
) ? L1_CACHE_BYTES : 2 * L1_CACHE_BYTES) - sizeof(struct xps_map)   \
>>       / sizeof(u16))
> But it indicates the whole kernel has the architectural assumption th=
at
> L1_CACHE_BYTES >=3D 32, so I don't think we should be trying to chang=
e
> that.
However, there are already at least two other arches with L1_CACHE_BYTE=
S=20
=3D 16.

Dave

--=20
John David Anglin  dave.anglin@bell.net

--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
 in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Helge Deller]

Hi Tom & David,

I've queued-up a patch for the parisc architecture which reduces L1_CAC=
HE_BYTES from 32 to 16:
  https://patchwork.kernel.org/patch/7399291/

But this change will break the kernel build like this:

In file included from net/core/dev.c:92:0:
net/core/dev.c: In function =E2=80=98expand_xps_map=E2=80=99:
include/linux/netdevice.h:721:27: warning: overflow in implicit constan=
t conversion [-Woverflow]
   #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))=
 \
net/core/dev.c:1972:18: note: in expansion of macro =E2=80=98XPS_MIN_MA=
P_ALLOC=E2=80=99
   int alloc_len =3D XPS_MIN_MAP_ALLOC;

Do you see an easy way to fix this ?

Thanks,
Helge=20
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
 in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Tom Herbert]

On Thu, Oct 22, 2015 at 1:00 PM, Helge Deller <deller@gmx.de> wrote:
> Hi Tom & David,
>
> I've queued-up a patch for the parisc architecture which reduces L1_C=
ACHE_BYTES from 32 to 16:
>   https://patchwork.kernel.org/patch/7399291/
>
> But this change will break the kernel build like this:
>
> In file included from net/core/dev.c:92:0:
> net/core/dev.c: In function =E2=80=98expand_xps_map=E2=80=99:
> include/linux/netdevice.h:721:27: warning: overflow in implicit const=
ant conversion [-Woverflow]
>    #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map=
)) \
> net/core/dev.c:1972:18: note: in expansion of macro =E2=80=98XPS_MIN_=
MAP_ALLOC=E2=80=99
>    int alloc_len =3D XPS_MIN_MAP_ALLOC;
>
> Do you see an easy way to fix this ?
>
How about

 #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - ((sizeof(struct xps_map)
% L1_CACHE_BYTES)) \

Tom

> Thanks,
> Helge
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Helge Deller]

On 22.10.2015 23:37, Tom Herbert wrote:
> On Thu, Oct 22, 2015 at 1:00 PM, Helge Deller <deller@gmx.de> wrote:
>> Hi Tom & David,
>>
>> I've queued-up a patch for the parisc architecture which reduces L1_=
CACHE_BYTES from 32 to 16:
>>   https://patchwork.kernel.org/patch/7399291/
>>
>> But this change will break the kernel build like this:
>>
>> In file included from net/core/dev.c:92:0:
>> net/core/dev.c: In function =E2=80=98expand_xps_map=E2=80=99:
>> include/linux/netdevice.h:721:27: warning: overflow in implicit cons=
tant conversion [-Woverflow]
>>    #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_ma=
p)) \
>> net/core/dev.c:1972:18: note: in expansion of macro =E2=80=98XPS_MIN=
_MAP_ALLOC=E2=80=99
>>    int alloc_len =3D XPS_MIN_MAP_ALLOC;
>>
>> Do you see an easy way to fix this ?
>>
> How about
>=20
>  #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - ((sizeof(struct xps_map=
)
> % L1_CACHE_BYTES)) \

The full line would then be:
#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - (sizeof(struct xps_map) % =
L1_CACHE_BYTES)) / sizeof(u16))

The only problem I see with this is, that XPS_MIN_MAP_ALLOC might becom=
e zero.
In that case the call to kzalloc_node() in expand_xps_map() doesn't all=
ocate any memory for the queues.

Helge
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
 in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Tom Herbert]

On Fri, Oct 23, 2015 at 12:21 PM, Helge Deller <deller@gmx.de> wrote:
> On 22.10.2015 23:37, Tom Herbert wrote:
>> On Thu, Oct 22, 2015 at 1:00 PM, Helge Deller <deller@gmx.de> wrote:
>>> Hi Tom & David,
>>>
>>> I've queued-up a patch for the parisc architecture which reduces L1=
_CACHE_BYTES from 32 to 16:
>>>   https://patchwork.kernel.org/patch/7399291/
>>>
>>> But this change will break the kernel build like this:
>>>
>>> In file included from net/core/dev.c:92:0:
>>> net/core/dev.c: In function =E2=80=98expand_xps_map=E2=80=99:
>>> include/linux/netdevice.h:721:27: warning: overflow in implicit con=
stant conversion [-Woverflow]
>>>    #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_m=
ap)) \
>>> net/core/dev.c:1972:18: note: in expansion of macro =E2=80=98XPS_MI=
N_MAP_ALLOC=E2=80=99
>>>    int alloc_len =3D XPS_MIN_MAP_ALLOC;
>>>
>>> Do you see an easy way to fix this ?
>>>
>> How about
>>
>>  #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - ((sizeof(struct xps_ma=
p)
>> % L1_CACHE_BYTES)) \
>
> The full line would then be:
> #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - (sizeof(struct xps_map) =
% L1_CACHE_BYTES)) / sizeof(u16))
>
> The only problem I see with this is, that XPS_MIN_MAP_ALLOC might bec=
ome zero.
> In that case the call to kzalloc_node() in expand_xps_map() doesn't a=
llocate any memory for the queues.
>
I believe this wouldn't ever be zero (add 1 in to avoid result is 1 /
2 possibility):

#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES + 1 - (sizeof(struct
xps_map) % L1_CACHE_BYTES)) / sizeof(u16))

Tom

> Helge
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
 in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Eric Dumazet]

On Thu, 2015-10-22 at 22:00 +0200, Helge Deller wrote:
> Hi Tom & David,
>=20
> I've queued-up a patch for the parisc architecture which reduces L1_C=
ACHE_BYTES from 32 to 16:
>   https://patchwork.kernel.org/patch/7399291/
>=20
> But this change will break the kernel build like this:
>=20
> In file included from net/core/dev.c:92:0:
> net/core/dev.c: In function =E2=80=98expand_xps_map=E2=80=99:
> include/linux/netdevice.h:721:27: warning: overflow in implicit const=
ant conversion [-Woverflow]
>    #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map=
)) \
> net/core/dev.c:1972:18: note: in expansion of macro =E2=80=98XPS_MIN_=
MAP_ALLOC=E2=80=99
>    int alloc_len =3D XPS_MIN_MAP_ALLOC;
>=20
> Do you see an easy way to fix this ?


Using L2_CACHE_BYTES would be better, but it unfortunately does not
exist.

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Helge Deller]

On 22.10.2015 23:50, Eric Dumazet wrote:
> On Thu, 2015-10-22 at 22:00 +0200, Helge Deller wrote:
>> Hi Tom & David,
>>
>> I've queued-up a patch for the parisc architecture which reduces L1_=
CACHE_BYTES from 32 to 16:
>>   https://patchwork.kernel.org/patch/7399291/
>>
>> But this change will break the kernel build like this:
>>
>> In file included from net/core/dev.c:92:0:
>> net/core/dev.c: In function =E2=80=98expand_xps_map=E2=80=99:
>> include/linux/netdevice.h:721:27: warning: overflow in implicit cons=
tant conversion [-Woverflow]
>>    #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_ma=
p)) \
>> net/core/dev.c:1972:18: note: in expansion of macro =E2=80=98XPS_MIN=
_MAP_ALLOC=E2=80=99
>>    int alloc_len =3D XPS_MIN_MAP_ALLOC;
>>
>> Do you see an easy way to fix this ?
>=20
>=20
> Using L2_CACHE_BYTES would be better, but it unfortunately does not
> exist.

Then, how about simply changing it to twice of L1_CACHE_BYTES ?

#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)=
) / sizeof(u16))

Helge

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Eric Dumazet]

On Fri, 2015-10-23 at 21:25 +0200, Helge Deller wrote:

> Then, how about simply changing it to twice of L1_CACHE_BYTES ?
> 
> #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) / sizeof(u16))


Seems good to me.

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Helge Deller]

* Eric Dumazet <eric.dumazet@gmail.com>:
> On Fri, 2015-10-23 at 21:25 +0200, Helge Deller wrote:
> 
> > Then, how about simply changing it to twice of L1_CACHE_BYTES ?
> > 
> > #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) / sizeof(u16))
> 
> 
> Seems good to me.

Great!

Can you then maybe give me an Acked-by or signed-off for the patch below?
It further adds a compile-time check to avoid that XPS_MIN_MAP_ALLOC
gets calculated to zero on any architecture - otherwise no queues would
be allocated. 

In addition I would like to push it for v4.3 then through my parisc-tree
(after keeping it in for-next for 1-2 days), together with the patch
which reduces L1_CACHE_BYTES to 16 on parisc.
Would that be OK too? 

Thanks!
Helge


[PATCH] net/xps: Increase initial number of xps queues

Increase the number of initial allocated xps queues, so that the initial record
allocates twice the size of L1_CACHE_BYTES bytes.

This change is needed to copy with architectures where L1_CACHE_BYTES is
defined to equal or less than 16 bytes.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2d15e38..d152788 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -718,7 +718,7 @@ struct xps_map {
 	u16 queues[0];
 };
 #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(u16)))
-#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))	\
+#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) \
     / sizeof(u16))
 
 /*
diff --git a/net/core/dev.c b/net/core/dev.c
index 6bb6470..f6d6dd1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1972,6 +1972,8 @@ static struct xps_map *expand_xps_map(struct xps_map *map,
 	int alloc_len = XPS_MIN_MAP_ALLOC;
 	int i, pos;
 
+	BUILD_BUG_ON(XPS_MIN_MAP_ALLOC == 0);
+
 	for (pos = 0; map && pos < map->len; pos++) {
 		if (map->queues[pos] != index)
 			continue;

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Helge Deller]

On 23.10.2015 23:08, Helge Deller wrote:
> * Eric Dumazet <eric.dumazet@gmail.com>:
>> On Fri, 2015-10-23 at 21:25 +0200, Helge Deller wrote:
>>
>>> Then, how about simply changing it to twice of L1_CACHE_BYTES ?
>>>
>>> #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) / sizeof(u16))
>>
>>
>> Seems good to me.
> 
> Great!
> 
> Can you then maybe give me an Acked-by or signed-off for the patch below?
> It further adds a compile-time check to avoid that XPS_MIN_MAP_ALLOC
> gets calculated to zero on any architecture - otherwise no queues would
> be allocated. 
> 
> In addition I would like to push it for v4.3 then through my parisc-tree
> (after keeping it in for-next for 1-2 days), together with the patch
> which reduces L1_CACHE_BYTES to 16 on parisc.
> Would that be OK too? 
> 
> Thanks!
> Helge
> 
> 
> [PATCH] net/xps: Increase initial number of xps queues
> 
> Increase the number of initial allocated xps queues, so that the initial record
> allocates twice the size of L1_CACHE_BYTES bytes.
> 
> This change is needed to copy with architectures where L1_CACHE_BYTES is

s/copy/cope/g


> defined to equal or less than 16 bytes.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 2d15e38..d152788 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -718,7 +718,7 @@ struct xps_map {
>  	u16 queues[0];
>  };
>  #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(u16)))
> -#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))	\
> +#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) \
>      / sizeof(u16))
>  
>  /*
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6bb6470..f6d6dd1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1972,6 +1972,8 @@ static struct xps_map *expand_xps_map(struct xps_map *map,
>  	int alloc_len = XPS_MIN_MAP_ALLOC;
>  	int i, pos;
>  
> +	BUILD_BUG_ON(XPS_MIN_MAP_ALLOC == 0);
> +
>  	for (pos = 0; map && pos < map->len; pos++) {
>  		if (map->queues[pos] != index)
>  			continue;
> 

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Eric Dumazet]

On Fri, 2015-10-23 at 23:08 +0200, Helge Deller wrote:

> Can you then maybe give me an Acked-by or signed-off for the patch below?
> It further adds a compile-time check to avoid that XPS_MIN_MAP_ALLOC
> gets calculated to zero on any architecture - otherwise no queues would
> be allocated. 
> 
> In addition I would like to push it for v4.3 then through my parisc-tree
> (after keeping it in for-next for 1-2 days), together with the patch
> which reduces L1_CACHE_BYTES to 16 on parisc.
> Would that be OK too? 

Sure !

Acked-by: Eric Dumazet <edumazet@google.com>

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Alexander Duyck]

On 10/23/2015 02:08 PM, Helge Deller wrote:
> * Eric Dumazet <eric.dumazet@gmail.com>:
>> On Fri, 2015-10-23 at 21:25 +0200, Helge Deller wrote:
>>
>>> Then, how about simply changing it to twice of L1_CACHE_BYTES ?
>>>
>>> #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) / sizeof(u16))
>>
>>
>> Seems good to me.
>
> Great!
>
> Can you then maybe give me an Acked-by or signed-off for the patch below?
> It further adds a compile-time check to avoid that XPS_MIN_MAP_ALLOC
> gets calculated to zero on any architecture - otherwise no queues would
> be allocated.
>
> In addition I would like to push it for v4.3 then through my parisc-tree
> (after keeping it in for-next for 1-2 days), together with the patch
> which reduces L1_CACHE_BYTES to 16 on parisc.
> Would that be OK too?
>
> Thanks!
> Helge
>
>
> [PATCH] net/xps: Increase initial number of xps queues
>
> Increase the number of initial allocated xps queues, so that the initial record
> allocates twice the size of L1_CACHE_BYTES bytes.
>
> This change is needed to copy with architectures where L1_CACHE_BYTES is
> defined to equal or less than 16 bytes.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 2d15e38..d152788 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -718,7 +718,7 @@ struct xps_map {
>   	u16 queues[0];
>   };
>   #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(u16)))
> -#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))	\
> +#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) \
>       / sizeof(u16))
>
>   /*
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6bb6470..f6d6dd1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1972,6 +1972,8 @@ static struct xps_map *expand_xps_map(struct xps_map *map,
>   	int alloc_len = XPS_MIN_MAP_ALLOC;
>   	int i, pos;
>
> +	BUILD_BUG_ON(XPS_MIN_MAP_ALLOC == 0);
> +
>   	for (pos = 0; map && pos < map->len; pos++) {
>   		if (map->queues[pos] != index)
>   			continue;
>
>

Rather then leaving a potential bug you could probably rewrite the macro 
so that it will give you at least 1.

All you need to do is something like the following
#define XPS_MIN_MAP_ALLOC \
	((L1_CACHE_ALIGN(offsetof(struct xps_map, queue[1])) - \
	  sizeof(struct xps_map)) / sizeof(u16))

That should give you at least an XPS_MIN_MAP_ALLOC of 1.

- Alex

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Helge Deller]

On 24.10.2015 00:00, Alexander Duyck wrote:
> On 10/23/2015 02:08 PM, Helge Deller wrote:
>> * Eric Dumazet <eric.dumazet@gmail.com>:
>>> On Fri, 2015-10-23 at 21:25 +0200, Helge Deller wrote:
>>>
>>>> Then, how about simply changing it to twice of L1_CACHE_BYTES ?
>>>>
>>>> #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) / sizeof(u16))
>>>
>>>
>>> Seems good to me.
>>
>> Great!
>>
>> Can you then maybe give me an Acked-by or signed-off for the patch below?
>> It further adds a compile-time check to avoid that XPS_MIN_MAP_ALLOC
>> gets calculated to zero on any architecture - otherwise no queues would
>> be allocated.
>>
>> In addition I would like to push it for v4.3 then through my parisc-tree
>> (after keeping it in for-next for 1-2 days), together with the patch
>> which reduces L1_CACHE_BYTES to 16 on parisc.
>> Would that be OK too?
>>
>> Thanks!
>> Helge
>>
>>
>> [PATCH] net/xps: Increase initial number of xps queues
>>
>> Increase the number of initial allocated xps queues, so that the initial record
>> allocates twice the size of L1_CACHE_BYTES bytes.
>>
>> This change is needed to copy with architectures where L1_CACHE_BYTES is
>> defined to equal or less than 16 bytes.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 2d15e38..d152788 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -718,7 +718,7 @@ struct xps_map {
>>       u16 queues[0];
>>   };
>>   #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(u16)))
>> -#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))    \
>> +#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) \
>>       / sizeof(u16))
>>
>>   /*
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 6bb6470..f6d6dd1 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1972,6 +1972,8 @@ static struct xps_map *expand_xps_map(struct xps_map *map,
>>       int alloc_len = XPS_MIN_MAP_ALLOC;
>>       int i, pos;
>>
>> +    BUILD_BUG_ON(XPS_MIN_MAP_ALLOC == 0);
>> +
>>       for (pos = 0; map && pos < map->len; pos++) {
>>           if (map->queues[pos] != index)
>>               continue;
>>
>>
> 
> Rather then leaving a potential bug you could probably rewrite the macro so that it will give you at least 1.
> 
> All you need to do is something like the following
> #define XPS_MIN_MAP_ALLOC \
>     ((L1_CACHE_ALIGN(offsetof(struct xps_map, queue[1])) - \
>       sizeof(struct xps_map)) / sizeof(u16))
> 
> That should give you at least an XPS_MIN_MAP_ALLOC of 1.

Yes, good idea!
What makes me wonder though (because I have no idea about the XPS code/layer):
How likely is it, that more than 1 (e.g. minimum "X") queues are needed? 
E.g. if a typical system needs at least 3 queues, then doesn't it make sense to allocate
at least 3 initially by using queue[3] in your proposed patch above ?
What would "X" be then?

Helge

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Alexander Duyck]

On 10/23/2015 03:17 PM, Helge Deller wrote:
> On 24.10.2015 00:00, Alexander Duyck wrote:
>> On 10/23/2015 02:08 PM, Helge Deller wrote:
>>> * Eric Dumazet <eric.dumazet@gmail.com>:
>>>> On Fri, 2015-10-23 at 21:25 +0200, Helge Deller wrote:
>>>>
>>>>> Then, how about simply changing it to twice of L1_CACHE_BYTES ?
>>>>>
>>>>> #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) / sizeof(u16))
>>>>
>>>>
>>>> Seems good to me.
>>>
>>> Great!
>>>
>>> Can you then maybe give me an Acked-by or signed-off for the patch below?
>>> It further adds a compile-time check to avoid that XPS_MIN_MAP_ALLOC
>>> gets calculated to zero on any architecture - otherwise no queues would
>>> be allocated.
>>>
>>> In addition I would like to push it for v4.3 then through my parisc-tree
>>> (after keeping it in for-next for 1-2 days), together with the patch
>>> which reduces L1_CACHE_BYTES to 16 on parisc.
>>> Would that be OK too?
>>>
>>> Thanks!
>>> Helge
>>>
>>>
>>> [PATCH] net/xps: Increase initial number of xps queues
>>>
>>> Increase the number of initial allocated xps queues, so that the initial record
>>> allocates twice the size of L1_CACHE_BYTES bytes.
>>>
>>> This change is needed to copy with architectures where L1_CACHE_BYTES is
>>> defined to equal or less than 16 bytes.
>>>
>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index 2d15e38..d152788 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>>> @@ -718,7 +718,7 @@ struct xps_map {
>>>        u16 queues[0];
>>>    };
>>>    #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(u16)))
>>> -#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))    \
>>> +#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) \
>>>        / sizeof(u16))
>>>
>>>    /*
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 6bb6470..f6d6dd1 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -1972,6 +1972,8 @@ static struct xps_map *expand_xps_map(struct xps_map *map,
>>>        int alloc_len = XPS_MIN_MAP_ALLOC;
>>>        int i, pos;
>>>
>>> +    BUILD_BUG_ON(XPS_MIN_MAP_ALLOC == 0);
>>> +
>>>        for (pos = 0; map && pos < map->len; pos++) {
>>>            if (map->queues[pos] != index)
>>>                continue;
>>>
>>>
>>
>> Rather then leaving a potential bug you could probably rewrite the macro so that it will give you at least 1.
>>
>> All you need to do is something like the following
>> #define XPS_MIN_MAP_ALLOC \
>>      ((L1_CACHE_ALIGN(offsetof(struct xps_map, queue[1])) - \
>>        sizeof(struct xps_map)) / sizeof(u16))
>>
>> That should give you at least an XPS_MIN_MAP_ALLOC of 1.
>
> Yes, good idea!
> What makes me wonder though (because I have no idea about the XPS code/layer):
> How likely is it, that more than 1 (e.g. minimum "X") queues are needed?
> E.g. if a typical system needs at least 3 queues, then doesn't it make sense to allocate
> at least 3 initially by using queue[3] in your proposed patch above ?
> What would "X" be then?

The question I would have is in how many cases it it likely that 
somebody would enable this feature and point a given CPU at more than 
one queue.  I know the Intel drivers that make use of XPS tend to do a 
1:1 mapping for their ATR feature.  I would think if anything most CPUs 
would probably be mapped many:1, but you probably won't have all that 
many cases where it is 1:many or many:many.

I'd say starting with at least 1 should be fine.  Worst case scenario is 
we have to make a couple more calls to expand_xps_map which will likely 
occur as a slow path and infrequent event anyway.

- Alex

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Helge Deller]

* Alexander Duyck <alexander.duyck@gmail.com>:
> On 10/23/2015 03:17 PM, Helge Deller wrote:
> >On 24.10.2015 00:00, Alexander Duyck wrote:
> >>On 10/23/2015 02:08 PM, Helge Deller wrote:
> >>>* Eric Dumazet <eric.dumazet@gmail.com>:
> >>>>On Fri, 2015-10-23 at 21:25 +0200, Helge Deller wrote:
> >>>>
> >>>>>Then, how about simply changing it to twice of L1_CACHE_BYTES ?
> >>>>>
> >>>>>#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) / sizeof(u16))
> >>>>
> >>>>
> >>>>Seems good to me.
> >>>
> >>>Great!
> >>>
> >>>Can you then maybe give me an Acked-by or signed-off for the patch below?
> >>>It further adds a compile-time check to avoid that XPS_MIN_MAP_ALLOC
> >>>gets calculated to zero on any architecture - otherwise no queues would
> >>>be allocated.
> >>>
> >>>In addition I would like to push it for v4.3 then through my parisc-tree
> >>>(after keeping it in for-next for 1-2 days), together with the patch
> >>>which reduces L1_CACHE_BYTES to 16 on parisc.
> >>>Would that be OK too?
> >>>
> >>>Thanks!
> >>>Helge
> >>>
> >>>
> >>>[PATCH] net/xps: Increase initial number of xps queues
> >>>
> >>>Increase the number of initial allocated xps queues, so that the initial record
> >>>allocates twice the size of L1_CACHE_BYTES bytes.
> >>>
> >>>This change is needed to copy with architectures where L1_CACHE_BYTES is
> >>>defined to equal or less than 16 bytes.
> >>>
> >>>Signed-off-by: Helge Deller <deller@gmx.de>
> >>>
> >>>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >>>index 2d15e38..d152788 100644
> >>>--- a/include/linux/netdevice.h
> >>>+++ b/include/linux/netdevice.h
> >>>@@ -718,7 +718,7 @@ struct xps_map {
> >>>       u16 queues[0];
> >>>   };
> >>>   #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(u16)))
> >>>-#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))    \
> >>>+#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) \
> >>>       / sizeof(u16))
> >>>
> >>>   /*
> >>>diff --git a/net/core/dev.c b/net/core/dev.c
> >>>index 6bb6470..f6d6dd1 100644
> >>>--- a/net/core/dev.c
> >>>+++ b/net/core/dev.c
> >>>@@ -1972,6 +1972,8 @@ static struct xps_map *expand_xps_map(struct xps_map *map,
> >>>       int alloc_len = XPS_MIN_MAP_ALLOC;
> >>>       int i, pos;
> >>>
> >>>+    BUILD_BUG_ON(XPS_MIN_MAP_ALLOC == 0);
> >>>+
> >>>       for (pos = 0; map && pos < map->len; pos++) {
> >>>           if (map->queues[pos] != index)
> >>>               continue;
> >>>
> >>>
> >>
> >>Rather then leaving a potential bug you could probably rewrite the macro so that it will give you at least 1.
> >>
> >>All you need to do is something like the following
> >>#define XPS_MIN_MAP_ALLOC \
> >>     ((L1_CACHE_ALIGN(offsetof(struct xps_map, queue[1])) - \
> >>       sizeof(struct xps_map)) / sizeof(u16))
> >>
> >>That should give you at least an XPS_MIN_MAP_ALLOC of 1.
> >
> >Yes, good idea!
> >
> >What makes me wonder though (because I have no idea about the XPS code/layer):
> >How likely is it, that more than 1 (e.g. minimum "X") queues are needed?
> >E.g. if a typical system needs at least 3 queues, then doesn't it make sense to allocate
> >at least 3 initially by using queue[3] in your proposed patch above ?
> >What would "X" be then?
> 
> The question I would have is in how many cases it it likely that somebody
> would enable this feature and point a given CPU at more than one queue.  I
> know the Intel drivers that make use of XPS tend to do a 1:1 mapping for
> their ATR feature.  I would think if anything most CPUs would probably be
> mapped many:1, but you probably won't have all that many cases where it is
> 1:many or many:many.
> 
> I'd say starting with at least 1 should be fine.  Worst case scenario is we
> have to make a couple more calls to expand_xps_map which will likely occur
> as a slow path and infrequent event anyway.

Ok, can I get then the signed-off or acked-by from you for this patch?

Thanks,
Helge


[PATCH] net/xps: Fix calculation of initial number of xps queues

The existing code breaks on architectures where the L1 cache size
(L1_CACHE_BYTES) is smaller or equal the size of struct xps_map.

The new code ensures that we get at minimum one initial xps queue, or
even more as long as it fits into the next multiple of L1_CACHE_SIZE.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2d15e38..2212c82 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -718,8 +718,8 @@ struct xps_map {
 	u16 queues[0];
 };
 #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(u16)))
-#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))	\
-    / sizeof(u16))
+#define XPS_MIN_MAP_ALLOC ((L1_CACHE_ALIGN(offsetof(struct xps_map, queues[1])) \
+       - sizeof(struct xps_map)) / sizeof(u16))
 
 /*
  * This structure holds all XPS maps for device.  Maps are indexed by CPU.

Re: CONFIG_XPS depends on L1_CACHE_BYTES being greater than sizeof(struct xps_map)

[Alexander Duyck]

On 10/24/2015 07:43 AM, Helge Deller wrote:
> * Alexander Duyck <alexander.duyck@gmail.com>:
>> On 10/23/2015 03:17 PM, Helge Deller wrote:
>>> On 24.10.2015 00:00, Alexander Duyck wrote:
>>>> On 10/23/2015 02:08 PM, Helge Deller wrote:
>>>>> * Eric Dumazet <eric.dumazet@gmail.com>:
>>>>>> On Fri, 2015-10-23 at 21:25 +0200, Helge Deller wrote:
>>>>>>
>>>>>>> Then, how about simply changing it to twice of L1_CACHE_BYTES ?
>>>>>>>
>>>>>>> #define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) / sizeof(u16))
>>>>>>
>>>>>>
>>>>>> Seems good to me.
>>>>>
>>>>> Great!
>>>>>
>>>>> Can you then maybe give me an Acked-by or signed-off for the patch below?
>>>>> It further adds a compile-time check to avoid that XPS_MIN_MAP_ALLOC
>>>>> gets calculated to zero on any architecture - otherwise no queues would
>>>>> be allocated.
>>>>>
>>>>> In addition I would like to push it for v4.3 then through my parisc-tree
>>>>> (after keeping it in for-next for 1-2 days), together with the patch
>>>>> which reduces L1_CACHE_BYTES to 16 on parisc.
>>>>> Would that be OK too?
>>>>>
>>>>> Thanks!
>>>>> Helge
>>>>>
>>>>>
>>>>> [PATCH] net/xps: Increase initial number of xps queues
>>>>>
>>>>> Increase the number of initial allocated xps queues, so that the initial record
>>>>> allocates twice the size of L1_CACHE_BYTES bytes.
>>>>>
>>>>> This change is needed to copy with architectures where L1_CACHE_BYTES is
>>>>> defined to equal or less than 16 bytes.
>>>>>
>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>>
>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>> index 2d15e38..d152788 100644
>>>>> --- a/include/linux/netdevice.h
>>>>> +++ b/include/linux/netdevice.h
>>>>> @@ -718,7 +718,7 @@ struct xps_map {
>>>>>        u16 queues[0];
>>>>>    };
>>>>>    #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(u16)))
>>>>> -#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))    \
>>>>> +#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES * 2 - sizeof(struct xps_map)) \
>>>>>        / sizeof(u16))
>>>>>
>>>>>    /*
>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>>> index 6bb6470..f6d6dd1 100644
>>>>> --- a/net/core/dev.c
>>>>> +++ b/net/core/dev.c
>>>>> @@ -1972,6 +1972,8 @@ static struct xps_map *expand_xps_map(struct xps_map *map,
>>>>>        int alloc_len = XPS_MIN_MAP_ALLOC;
>>>>>        int i, pos;
>>>>>
>>>>> +    BUILD_BUG_ON(XPS_MIN_MAP_ALLOC == 0);
>>>>> +
>>>>>        for (pos = 0; map && pos < map->len; pos++) {
>>>>>            if (map->queues[pos] != index)
>>>>>                continue;
>>>>>
>>>>>
>>>>
>>>> Rather then leaving a potential bug you could probably rewrite the macro so that it will give you at least 1.
>>>>
>>>> All you need to do is something like the following
>>>> #define XPS_MIN_MAP_ALLOC \
>>>>      ((L1_CACHE_ALIGN(offsetof(struct xps_map, queue[1])) - \
>>>>        sizeof(struct xps_map)) / sizeof(u16))
>>>>
>>>> That should give you at least an XPS_MIN_MAP_ALLOC of 1.
>>>
>>> Yes, good idea!
>>>
>>> What makes me wonder though (because I have no idea about the XPS code/layer):
>>> How likely is it, that more than 1 (e.g. minimum "X") queues are needed?
>>> E.g. if a typical system needs at least 3 queues, then doesn't it make sense to allocate
>>> at least 3 initially by using queue[3] in your proposed patch above ?
>>> What would "X" be then?
>>
>> The question I would have is in how many cases it it likely that somebody
>> would enable this feature and point a given CPU at more than one queue.  I
>> know the Intel drivers that make use of XPS tend to do a 1:1 mapping for
>> their ATR feature.  I would think if anything most CPUs would probably be
>> mapped many:1, but you probably won't have all that many cases where it is
>> 1:many or many:many.
>>
>> I'd say starting with at least 1 should be fine.  Worst case scenario is we
>> have to make a couple more calls to expand_xps_map which will likely occur
>> as a slow path and infrequent event anyway.
>
> Ok, can I get then the signed-off or acked-by from you for this patch?
>
> Thanks,
> Helge
>
>
> [PATCH] net/xps: Fix calculation of initial number of xps queues
>
> The existing code breaks on architectures where the L1 cache size
> (L1_CACHE_BYTES) is smaller or equal the size of struct xps_map.
>
> The new code ensures that we get at minimum one initial xps queue, or
> even more as long as it fits into the next multiple of L1_CACHE_SIZE.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 2d15e38..2212c82 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -718,8 +718,8 @@ struct xps_map {
>   	u16 queues[0];
>   };
>   #define XPS_MAP_SIZE(_num) (sizeof(struct xps_map) + ((_num) * sizeof(u16)))
> -#define XPS_MIN_MAP_ALLOC ((L1_CACHE_BYTES - sizeof(struct xps_map))	\
> -    / sizeof(u16))
> +#define XPS_MIN_MAP_ALLOC ((L1_CACHE_ALIGN(offsetof(struct xps_map, queues[1])) \
> +       - sizeof(struct xps_map)) / sizeof(u16))
>
>   /*
>    * This structure holds all XPS maps for device.  Maps are indexed by CPU.
>

This looks good to me.

Acked-by: Alexander Duyck <aduyck@mirantis.com>