lkml.org 
[lkml]   [2019]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH net-next 1/6] net: core: dev_addr_lists: add VID to device address
On Wed, Feb 27, 2019 at 08:24:00PM -0800, Florian Fainelli wrote:
>On 2/26/2019 10:45 AM, Ivan Khoronzhuk wrote:
>> Despite this is supposed to be used for Ethernet VLANs, not Ethernet
>> addresses with space for VID also can reuse this, so VID is considered
>> as virtual ID extension, not belonging strictly to Ethernet VLAN VIDs,
>> and overall change can be named individual virtual device filtering
>> (IVDF).
>>
>> This patch adds VID tag at the end of each address. The actual
>> reserved address size is 32 bytes. For Ethernet addresses with 6 bytes
>> long that's possible to add tag w/o increasing address size. Thus,
>> each address for the case has 32 - 6 = 26 bytes to hold additional
>> info, say VID for virtual device addresses.
>>
>> Therefore, when addresses are synced to the address list of parent
>> device the address list of latter can contain separate addresses for
>> virtual devices. It allows to track separate address tables for
>> virtual devices if they present and the device can be placed on
>> any place of device tree as the address is propagated to to the end
>> real device thru *_sync()/ndo_set_rx_mode() APIs. Also it simplifies
>> handling VID addresses at real device when it supports IVDF.
>>
>> If parent device doesn't want to have virtual addresses in its address
>> space the vid_len has to be 0, thus its address space is "shrunk" to
>> the state as before this patch. For now it's 0 for every device. It
>> allows two devices with and w/o IVDF to be part of same bond device
>> for instance.
>>
>> The end real device supporting IVDF can retrieve VID tag from an
>> address and set it for a given virtual device only. By default, vid 0
>> is used for real devices to distinguish it from virtual addresses.
>>
>> See next patches to see how it's used.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>
>[snip]
>
>
>> @@ -1889,6 +1890,7 @@ struct net_device {
>> unsigned char perm_addr[MAX_ADDR_LEN];
>> unsigned char addr_assign_type;
>> unsigned char addr_len;
>> + unsigned char vid_len;
>
>Have not compiled or tested this patch series yet, but did you check
>that adding this member does not change the structure layout (you can
>use pahole for that purpose).

For ARM 32, on 1 hole less:
---------------------------

before (https://pastebin.com/DG1SVpFR):

/* size: 1344, cachelines: 21, members: 123 */
/* sum members: 1304, holes: 5, sum holes: 28 */
/* padding: 12 */
/* bit_padding: 31 bits */

after (https://pastebin.com/ZUMhxGkA):

/* size: 1344, cachelines: 21, members: 124 */
/* sum members: 1305, holes: 5, sum holes: 27 */
/* padding: 12 */
/* bit_padding: 31 bits */


For ARM 64, on 1 hole less:
---------------------------

before (https://pastebin.com/5CdTQWkc):

/* size: 2048, cachelines: 32, members: 120 */
/* sum members: 1972, holes: 7, sum holes: 48 */
/* padding: 28 */
/* bit_padding: 31 bits */

after (https://pastebin.com/32ktb1iV):

/* size: 2048, cachelines: 32, members: 121 */
/* sum members: 1973, holes: 7, sum holes: 47 */
/* padding: 28 */
/* bit_padding: 31 bits */

Looks Ok, but it depends on configuration ...

>
>> unsigned short neigh_priv_len;
>> unsigned short dev_id;
>> unsigned short dev_port;
>> @@ -4141,8 +4143,10 @@ int dev_addr_init(struct net_device *dev);
>>
>> /* Functions used for unicast addresses handling */
>> int dev_uc_add(struct net_device *dev, const unsigned char *addr);
>> +int dev_vid_uc_add(struct net_device *dev, const unsigned char *addr);
>> int dev_uc_add_excl(struct net_device *dev, const unsigned char *addr);
>> int dev_uc_del(struct net_device *dev, const unsigned char *addr);
>> +int dev_vid_uc_del(struct net_device *dev, const unsigned char *addr);
>> int dev_uc_sync(struct net_device *to, struct net_device *from);
>> int dev_uc_sync_multiple(struct net_device *to, struct net_device *from);
>> void dev_uc_unsync(struct net_device *to, struct net_device *from);
>> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
>> index a6723b306717..e3c80e044b8c 100644
>> --- a/net/core/dev_addr_lists.c
>> +++ b/net/core/dev_addr_lists.c
>> @@ -545,6 +545,26 @@ int dev_addr_del(struct net_device *dev, const unsigned char *addr,
>> }
>> EXPORT_SYMBOL(dev_addr_del);
>>
>> +static int get_addr_len(struct net_device *dev)
>> +{
>> + return dev->addr_len + dev->vid_len;
>> +}
>> +
>> +static int set_vid_addr(struct net_device *dev, const unsigned char *addr,
>> + unsigned char *naddr)
>
>Having some kernel doc comments here would be nice to indicate that the
>return value is dev->addr_len, it was not obvious until I saw in the
>next function how you used it.

Agree

>
>> +{
>> + int i;
>> +
>> + if (!dev->vid_len)
>> + return dev->addr_len;
>> +
>> + memcpy(naddr, addr, dev->addr_len);
>> + for (i = 0; i < dev->vid_len; i++)
>> + naddr[dev->addr_len + i] = 0;
>
>memset(naddr + dev->addr_len, 0, dev->vid_len) would be more compact and
>maybe a little less error prone too?

Yes, would be

--
Regards,
Ivan Khoronzhuk
\
 
 \ /
  Last update: 2019-03-01 13:21    [W:0.424 / U:0.788 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site