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

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_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 ‘expand_xps_map’:
include/linux/netdevice.h:721:27: warning: overflow in implicit constant 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 ‘XPS_MIN_MAP_ALLOC’
   int alloc_len = XPS_MIN_MAP_ALLOC;

Do you see an easy way to fix this ?

Thanks,
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 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 ‘expand_xps_map’:
> include/linux/netdevice.h:721:27: warning: overflow in implicit constant 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 ‘XPS_MIN_MAP_ALLOC’
>    int alloc_len = 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)

[Eric Dumazet]

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 ‘expand_xps_map’:
> include/linux/netdevice.h:721:27: warning: overflow in implicit constant 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 ‘XPS_MIN_MAP_ALLOC’
>    int alloc_len = XPS_MIN_MAP_ALLOC;
> 
> 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: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 ‘expand_xps_map’:
>> include/linux/netdevice.h:721:27: warning: overflow in implicit constant 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 ‘XPS_MIN_MAP_ALLOC’
>>    int alloc_len = 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)) \

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 become zero.
In that case the call to kzalloc_node() in expand_xps_map() doesn't allocate 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)

[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 ‘expand_xps_map’:
>> include/linux/netdevice.h:721:27: warning: overflow in implicit constant 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 ‘XPS_MIN_MAP_ALLOC’
>>    int alloc_len = XPS_MIN_MAP_ALLOC;
>>
>> Do you see an easy way to fix this ?
> 
> 
> 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)

[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 ‘expand_xps_map’:
>>> include/linux/netdevice.h:721:27: warning: overflow in implicit constant 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 ‘XPS_MIN_MAP_ALLOC’
>>>    int alloc_len = 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)) \
>
> 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 become zero.
> In that case the call to kzalloc_node() in expand_xps_map() doesn't allocate 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)

[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>