Re: [PATCH net-next 1/4] soreuseport: define reuseport groups

[David Miller]

From: Craig Gallek 
Date: Tue, 22 Dec 2015 16:58:11 -0500

> On Tue, Dec 22, 2015 at 4:40 PM, David Miller  wrote:
>> From: Craig Gallek 
>> Date: Tue, 22 Dec 2015 15:05:07 -0500
>>
>>> + for (i = 0; i < reuse->num_socks; i++) {
>>> + if (reuse->socks[i] == sk) {
>>> + reuse->socks[i] = reuse->socks[reuse->num_socks - 1];
>>> + reuse->num_socks--;
>>> + if (reuse->num_socks == 0)
>>> + kfree_rcu(reuse, rcu);
>>> + break;
>>> + }
>>> + }
>>
>> Don't you need to memmove() the entire rest of the array down one slot
>> when you hit the matching 'sk' in there?  I can't see how it can work
>> to only move one entry down.
> It moves the last element in the list into the slot that was just
> emptied.  You could argue that this may cause unexpected changes in
> the index -> socket mapping observed by the user, but I'm not sure
> making many socket indexes change (by sliding everything down one)
> when one is removed is a desirable behavior either.  I don't have a
> strong opinion either way though...

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

Re: [PATCH net-next 1/4] soreuseport: define reuseport groups

[kbuild test robot]

Hi Craig,

[auto build test ERROR on net-next/master]

url:
https://github.com/0day-ci/linux/commits/Craig-Gallek/Faster-SO_REUSEPORT/20151223-040911
config: arm-mvebu_v7_defconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

Note: the linux-review/Craig-Gallek/Faster-SO_REUSEPORT/20151223-040911 HEAD 
660edb1fa7e1a2bf71ef9cbf4555cda4af95ea58 builds fine.
  It only hurts bisectibility.

All errors (new ones prefixed by >>):

   In file included from include/linux/spinlock_types.h:13:0,
from include/net/sock_reuseport.h:4,
from net/core/sock_reuseport.c:7:
>> arch/arm/include/asm/spinlock_types.h:12:3: error: unknown type name 'u32'
  u32 slock;
  ^
>> arch/arm/include/asm/spinlock_types.h:18:4: error: unknown type name 'u16'
   u16 owner;
   ^
   arch/arm/include/asm/spinlock_types.h:19:4: error: unknown type name 'u16'
   u16 next;
   ^
   arch/arm/include/asm/spinlock_types.h:28:2: error: unknown type name 'u32'
 u32 lock;
 ^

vim +/u32 +12 arch/arm/include/asm/spinlock_types.h

fb1c8f93 include/asm-arm/spinlock_types.h  Ingo Molnar 2005-09-10   6  
#endif
fb1c8f93 include/asm-arm/spinlock_types.h  Ingo Molnar 2005-09-10   7  
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06   8  
#define TICKET_SHIFT 16
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06   9  
fb1c8f93 include/asm-arm/spinlock_types.h  Ingo Molnar 2005-09-10  10  
typedef struct {
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  11   
union {
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06 @12   
u32 slock;
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  13   
struct __raw_tickets {
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  14  
#ifdef __ARMEB__
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  15   
u16 next;
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  16   
u16 owner;
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  17  #else
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06 @18   
u16 owner;
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  19   
u16 next;
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  20  
#endif
546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  21   
} tickets;

:: The code at line 12 was first introduced by commit
:: 546c2896a42202dbc7d02f7c6ec9948ac1bf511b ARM: 7446/1: spinlock: use 
ticket algorithm for ARMv6+ locking implementation

:: TO: Will Deacon 
:: CC: Russell King 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH net-next 1/4] soreuseport: define reuseport groups

[David Miller]

From: Craig Gallek 
Date: Tue, 22 Dec 2015 15:05:07 -0500

> + for (i = 0; i < reuse->num_socks; i++) {
> + if (reuse->socks[i] == sk) {
> + reuse->socks[i] = reuse->socks[reuse->num_socks - 1];
> + reuse->num_socks--;
> + if (reuse->num_socks == 0)
> + kfree_rcu(reuse, rcu);
> + break;
> + }
> + }

Don't you need to memmove() the entire rest of the array down one slot
when you hit the matching 'sk' in there?  I can't see how it can work
to only move one entry down.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 1/4] soreuseport: define reuseport groups

[Craig Gallek]

On Tue, Dec 22, 2015 at 4:40 PM, David Miller  wrote:
> From: Craig Gallek 
> Date: Tue, 22 Dec 2015 15:05:07 -0500
>
>> + for (i = 0; i < reuse->num_socks; i++) {
>> + if (reuse->socks[i] == sk) {
>> + reuse->socks[i] = reuse->socks[reuse->num_socks - 1];
>> + reuse->num_socks--;
>> + if (reuse->num_socks == 0)
>> + kfree_rcu(reuse, rcu);
>> + break;
>> + }
>> + }
>
> Don't you need to memmove() the entire rest of the array down one slot
> when you hit the matching 'sk' in there?  I can't see how it can work
> to only move one entry down.
It moves the last element in the list into the slot that was just
emptied.  You could argue that this may cause unexpected changes in
the index -> socket mapping observed by the user, but I'm not sure
making many socket indexes change (by sliding everything down one)
when one is removed is a desirable behavior either.  I don't have a
strong opinion either way though...
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 1/4] soreuseport: define reuseport groups

[Craig Gallek]

On Tue, Dec 22, 2015 at 5:11 PM, kbuild test robot  wrote:
> Hi Craig,
>
> [auto build test ERROR on net-next/master]
>
> url:
> https://github.com/0day-ci/linux/commits/Craig-Gallek/Faster-SO_REUSEPORT/20151223-040911
> config: arm-mvebu_v7_defconfig (attached as .config)
> reproduce:
> wget 
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
>  -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=arm
>
> Note: the linux-review/Craig-Gallek/Faster-SO_REUSEPORT/20151223-040911 HEAD 
> 660edb1fa7e1a2bf71ef9cbf4555cda4af95ea58 builds fine.
>   It only hurts bisectibility.
>
> All errors (new ones prefixed by >>):
>
>In file included from include/linux/spinlock_types.h:13:0,
> from include/net/sock_reuseport.h:4,
> from net/core/sock_reuseport.c:7:
>>> arch/arm/include/asm/spinlock_types.h:12:3: error: unknown type name 'u32'
>   u32 slock;
>   ^
>>> arch/arm/include/asm/spinlock_types.h:18:4: error: unknown type name 'u16'
>u16 owner;
>^
>arch/arm/include/asm/spinlock_types.h:19:4: error: unknown type name 'u16'
>u16 next;
>^
>arch/arm/include/asm/spinlock_types.h:28:2: error: unknown type name 'u32'
>  u32 lock;
>  ^
>
> vim +/u32 +12 arch/arm/include/asm/spinlock_types.h
>
> fb1c8f93 include/asm-arm/spinlock_types.h  Ingo Molnar 2005-09-10   6  
> #endif
> fb1c8f93 include/asm-arm/spinlock_types.h  Ingo Molnar 2005-09-10   7
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06   8  
> #define TICKET_SHIFT 16
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06   9
> fb1c8f93 include/asm-arm/spinlock_types.h  Ingo Molnar 2005-09-10  10  
> typedef struct {
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  11 
>   union {
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06 @12 
>   u32 slock;
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  13 
>   struct __raw_tickets {
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  14  
> #ifdef __ARMEB__
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  15 
>   u16 next;
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  16 
>   u16 owner;
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  17  
> #else
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06 @18 
>   u16 owner;
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  19 
>   u16 next;
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  20  
> #endif
> 546c2896 arch/arm/include/asm/spinlock_types.h Will Deacon 2012-07-06  21 
>   } tickets;
>
> :: The code at line 12 was first introduced by commit
> :: 546c2896a42202dbc7d02f7c6ec9948ac1bf511b ARM: 7446/1: spinlock: use 
> ticket algorithm for ARMv6+ locking implementation
>
> :: TO: Will Deacon 
> :: CC: Russell King 
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation

ACK, I don't even need spinlock_types for this file anymore (though it
may be worth fixing the arm version to include linux/types.h).  Will
remove for v2
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html