[PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Volodymyr Bendiuga]

Hashtable will make it extremely faster when inserting fdb entries
into the forwarding database.

Signed-off-by: Volodymyr Bendiuga <volodymyr.bendiuga@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 431e954..407e6db 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -15,6 +15,8 @@
 #include <linux/if_vlan.h>
 #include <linux/irq.h>
 #include <linux/gpio/consumer.h>
+#include <linux/crc32.h>
+#include <linux/hashtable.h>
 
 #ifndef UINT64_MAX
 #define UINT64_MAX		(u64)(~((u64)0))
@@ -672,6 +674,16 @@ struct mv88e6xxx_info {
 	const struct mv88e6xxx_ops *ops;
 };
 
+struct pvec_tbl_entry {
+        struct hlist_node entry;
+        u32 key_crc32; /* key */
+        u16 pvec;
+        struct pvec_tbl_key {
+                u8 addr[ETH_ALEN];
+                u16 fid;
+        } key;
+};
+
 struct mv88e6xxx_atu_entry {
 	u16	fid;
 	u8	state;
@@ -736,6 +748,9 @@ struct mv88e6xxx_chip {
 
 	struct mv88e6xxx_priv_port	ports[DSA_MAX_PORTS];
 
+	/* This table hold a port vector for each multicast address */
+	DECLARE_HASHTABLE(pvec_tbl, 12);
+
 	/* A switch may have a GPIO line tied to its reset pin. Parse
 	 * this from the device tree, and use it before performing
 	 * switch soft reset.
-- 
2.7.4

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Andrew Lunn]

On Mon, Dec 12, 2016 at 02:39:18PM +0100, Volodymyr Bendiuga wrote:
> Hashtable will make it extremely faster when inserting fdb entries
> into the forwarding database.
> 
> Signed-off-by: Volodymyr Bendiuga <volodymyr.bendiuga@gmail.com>
> ---
>  drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
> index 431e954..407e6db 100644
> --- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
> +++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
> @@ -15,6 +15,8 @@
>  #include <linux/if_vlan.h>
>  #include <linux/irq.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/crc32.h>
> +#include <linux/hashtable.h>
>  
>  #ifndef UINT64_MAX
>  #define UINT64_MAX		(u64)(~((u64)0))
> @@ -672,6 +674,16 @@ struct mv88e6xxx_info {
>  	const struct mv88e6xxx_ops *ops;
>  };
>  
> +struct pvec_tbl_entry {

Please use the mv88e6xxx_ prefix.

       Andrew

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Andrew Lunn]

Hi Volodymyr

Any multi patch patchset should include a cover note. See git
format-patch --cover. This should explain the big picture what does
the patchset do, why etc.

Also, David sent out an email saying net-next was closed. While it is
closed, please send patches are RFC.

Thanks
	Andrew

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Vivien Didelot]

Hi Volodymyr,

Volodymyr Bendiuga <volodymyr.bendiuga@gmail.com> writes:

> Hashtable will make it extremely faster when inserting fdb entries
> into the forwarding database.

This is hard to follow. As Andrew correctly mentioned, when you have two
or more patches, please format them with --cover-letter, describe them
in patch 0 and send them all together, so that we get a single thread.

You are correct about the ATU get operations being slow. However, we
intentionally keep the driver stateless at the moment to keep it simple.

Unless speed is an issue to fix here, I am quite reluctant to add
caching to this driver yet. What is the issue you are having with the
ATU being slow?

Thanks,

	Vivien

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Andrew Lunn]

On Mon, Dec 12, 2016 at 04:22:13PM +0100, Volodymyr Bendiuga wrote:
> Hi,
> 
> I apologise for incorrectly formatted patch, I will fix and resend it.

Please don't resend with just the white space fixed. The memory model
is very important, using the fdb_prepare() call to allocate memory,
etc.

   Andrew

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Florian Fainelli]

On 12/12/2016 07:22 AM, Volodymyr Bendiuga wrote:
> Hi,
> 
> I apologise for incorrectly formatted patch, I will fix and resend it.
> The problem with the ATU right now is that it is too slow when inserting
> entries.
> When the OS boots up, it might insert some multicast entries into the
> atu (if
> they are preconfigured by user). I run a test with 10 mc entries being
> configured for
> each port (13 ports), and it took 15 seconds, which made system quite
> slow on responding to
> other commands, as it has been inserting mc entries. The implementation
> with hashtable
> made insert command for 13 ports and 10 entries per port about 700 msec
> long.


Just wondering how do you achieve such speed up? What part of using a
hashtable allows you not to write down all 10 MC entries across the
ports? I would assume that the number of MDIO (is that the bus you are
using here?) operations would be identical.

Seeing such a change makes me wonder if we should not try to push some
of this hashtable abstraction (provided that we agree we want it) at a
higher layer, like net/dsa/slave.c?

Thanks!
-- 
Florian

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Vivien Didelot]

Hi all,

Florian Fainelli <f.fainelli@gmail.com> writes:

> Seeing such a change makes me wonder if we should not try to push some
> of this hashtable abstraction (provided that we agree we want it) at a
> higher layer, like net/dsa/slave.c?

That is the major reason why I am reluctant to cache stuffs in drivers.

In most cases, we want the DSA drivers to be "stupid", as much stateless
as possible, simply implementing the supported DSA switch operations.

The DSA core then handles the generic logic of how switch fabrics should
behave, and thus all DSA drivers are consistent and benefit from this.

Thanks,

        Vivien

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Andrew Lunn]

On Mon, Dec 12, 2016 at 08:37:50AM -0800, Florian Fainelli wrote:
> On 12/12/2016 07:22 AM, Volodymyr Bendiuga wrote:
> > Hi,
> > 
> > I apologise for incorrectly formatted patch, I will fix and resend it.
> > The problem with the ATU right now is that it is too slow when inserting
> > entries.
> > When the OS boots up, it might insert some multicast entries into the
> > atu (if
> > they are preconfigured by user). I run a test with 10 mc entries being
> > configured for
> > each port (13 ports), and it took 15 seconds, which made system quite
> > slow on responding to
> > other commands, as it has been inserting mc entries. The implementation
> > with hashtable
> > made insert command for 13 ports and 10 entries per port about 700 msec
> > long.

Humm, it looks like we are doing the atu_get wrong. We are looking for
a specific MAC address. Yet we seem to be walking the whole table to
find it, rather than getting the hardware to do the search. 

The current code is:

static int mv88e6xxx_atu_get(struct mv88e6xxx_chip *chip, int fid,
                             const u8 *addr, struct mv88e6xxx_atu_entry *entry)
{
        struct mv88e6xxx_atu_entry next;
        int err;

        eth_broadcast_addr(next.mac);

        err = _mv88e6xxx_atu_mac_write(chip, next.mac);

We should be setting next.mac to one less than the address we are
looking for.

Volodymyr, please could you try that, and see how much of a speed up
you get.

There is another optimization which can be made. We only say there is
no such entry once we have reached the end of the table. But it will
return the entries in ascending order. So if the entry it returned is
bigger than what we are looking for, we can immediately abort the
search and say it does not exist.

   Andrew

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> Humm, it looks like we are doing the atu_get wrong. We are looking for
> a specific MAC address. Yet we seem to be walking the whole table to
> find it, rather than getting the hardware to do the search. 

We are not doing it wrong, the hardware does the search. A classic dump
of an ATU database consists of starting from the broadcast address
ff:ff:ff:ff:ff:ff and issuing GetNext operation until we reach back the
broadcast address. Only addresses in used are returned by GetNext, thus
dumping an empty database is completed in a single operation.

I implemented atu_get intentionally this way because it provides simpler
code, rather than doing arithmetic on MAC addresses (Unless I am unaware
of simple increment/decrement code.)

> The current code is:
>
> static int mv88e6xxx_atu_get(struct mv88e6xxx_chip *chip, int fid,
>                              const u8 *addr, struct mv88e6xxx_atu_entry *entry)
> {
>         struct mv88e6xxx_atu_entry next;
>         int err;
>
>         eth_broadcast_addr(next.mac);
>
>         err = _mv88e6xxx_atu_mac_write(chip, next.mac);
>
> We should be setting next.mac to one less than the address we are
> looking for.
>
> Volodymyr, please could you try that, and see how much of a speed up
> you get.
>
> There is another optimization which can be made. We only say there is
> no such entry once we have reached the end of the table. But it will
> return the entries in ascending order. So if the entry it returned is
> bigger than what we are looking for, we can immediately abort the
> search and say it does not exist.

However your two suggestions to optimize the lookup are correct. It'd be
interesting to see if that makes a significant difference or not.

Thanks,

        Vivien

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Andrew Lunn]

> Another interesting thing is that fdb dump operation is slow
> too. The more entries you have the slower it gets. We have
> implemented caching with timing there as well (I have not submitted
> such patch yet), which makes fdb dump much more pleasant in user
> space, the operation happens instantly, where regular fdb dump op
> might take few seconds.

I know some people won't like the added MDIO transactions, because
they are doing PTP and want low jitter when talking to the switch and
PHYs. As things are now, once the system is configured, the MDIO bus
is idle. Hence there is very low jitter for PTP. For your cache to be
useful, i assume you are refreshing it at regular intervals. So you
are adding a constant load, which i guess 99.9% of the timer is never
used because there is no user looking at the table, where as the PTP
load is used to keep the clocks synchronised.

   Andrew

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Andrew Lunn]

On Wed, Dec 14, 2016 at 11:03:14AM +0100, Volodymyr Bendiuga wrote:
> Hi,
> 
> I understand what you wrote, but we do not refresh anything
> at intervals. FDB dump works only on user request, i.e. user
> run bridge fdb dump command. What we did is just a smarter
> way of feeding the dump request with entries obtained from
> switchcore. Interesting thing is that fdb dump in switchcore
> reads all entries, and from those entries only one is returned at
> a time, others are discarded, because request comes as per port.

Ah, O.K.

> What we do is we dump entries from switchcore once, when the
> first dump request comes, save all of them in the cache, and then
> all following fdb dump requests for each port will be fed from the cache,
> so no more hardware dump operations. We set the cache to be valid for
> 2 seconds, but this could be adjusted and tweaked. So in our case
> we decrease the number of MDIO reads quite a lot.

> What we are thinking now, is that this logics could be moved
> to net/dsa layer, and maybe unified with fdb load logics, if possible.

We should check what the other switches do. Can they perform a dump
with the hardware performing the port filter? Those switches will not
benefit from such a cache. But they should also not suffer.

Combining it with load is a bigger question. Currently the drivers are
stateless. That makes the error handling very simple. We don't have to
worry about running out of memory, since we don't allocate any memory.

If we run out of memory for this dump cache, it is not serious, since
a dump does not result in state change. But if we are out of memory on
a load, we do have state changes. We have to deal with the complexity
of allocating resources in the _prepare call, and then use the
resource in the do call. I would much prefer to avoid this at first,
by optimising the atu get. If we don't see a significant speed up,
then we can consider this complex solution of a cache for load.

     Andrew

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Vivien Didelot]

Hi Volodymyr,

Volodymyr Bendiuga <volodymyr.bendiuga@gmail.com> writes:

> Hi Andrew,
>
> I have tested the approach you wrote in previous mails, the one
> with setting next.mac to address we are looking for -1. It seems
> to be as slow as the original implementation, unfortunately.

Hum, that is what I was expecting... The ATU GetNext operation
(alongside an ether_addr_equal() call) should be quite fast.

> We use 6097 and 6352 chips, and both of them can not do any port
> filtering in hardware for fdb dump operation. Seems like they would
> benefit from cache. But I am not sure about other switches.
>
> Does anyone know about such feature in other switches?

Marvell switches cannot filter ATU entries for a specific port, they
contain a port vector.

I guess Florian might answer for Broadcom switches, and John might
answer for Qualcomm switches.

In all cases *if caching is really needed*, I think it won't hurt to do
it in DSA core even if a switch support FDB dump operations on a
per-port basis, as Andrew mentioned.

Thanks,

        Vivien

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Florian Fainelli]

On 12/15/2016 09:21 AM, Vivien Didelot wrote:
> Hi Volodymyr,
> 
> Volodymyr Bendiuga <volodymyr.bendiuga@gmail.com> writes:
> 
>> Hi Andrew,
>>
>> I have tested the approach you wrote in previous mails, the one
>> with setting next.mac to address we are looking for -1. It seems
>> to be as slow as the original implementation, unfortunately.
> 
> Hum, that is what I was expecting... The ATU GetNext operation
> (alongside an ether_addr_equal() call) should be quite fast.
> 
>> We use 6097 and 6352 chips, and both of them can not do any port
>> filtering in hardware for fdb dump operation. Seems like they would
>> benefit from cache. But I am not sure about other switches.
>>
>> Does anyone know about such feature in other switches?
> 
> Marvell switches cannot filter ATU entries for a specific port, they
> contain a port vector.
> 
> I guess Florian might answer for Broadcom switches, and John might
> answer for Qualcomm switches.

For Broadcom switches, we use the ARL search and then apply software
filtering to discard entries that are not for the target port bridge fdb
show was called with.

> 
> In all cases *if caching is really needed*, I think it won't hurt to do
> it in DSA core even if a switch support FDB dump operations on a
> per-port basis, as Andrew mentioned.

Agreed, and there does not appear to be any need to new dsa_switch_ops
operations to be introduced?
-- 
Florian

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Vivien Didelot]

Florian Fainelli <f.fainelli@gmail.com> writes:

>> In all cases *if caching is really needed*, I think it won't hurt to do
>> it in DSA core even if a switch support FDB dump operations on a
>> per-port basis, as Andrew mentioned.
>
> Agreed, and there does not appear to be any need to new dsa_switch_ops
> operations to be introduced?

Nope.

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[John Crispin]

On 16/12/2016 10:24, Volodymyr Bendiuga wrote:
> Hi all,
> 
> Does this mean we all agree on implementing caching
> mechanism in net/dsa layer? If yes, then I can start
> working on it immediately.
> 
> Regards,
> Volodymyr

i think that the brcm approach is better as at least on QCA you can also
read auto learned values from the table which would get broken by a
caching mechanism. filtering inside the dump function or the code
calling the dump function seams a lot simpler and lightweight to me.

	John

> 
> On Thu, Dec 15, 2016 at 6:50 PM, Vivien Didelot
> <vivien.didelot@savoirfairelinux.com
> <mailto:vivien.didelot@savoirfairelinux.com>> wrote:
> 
>     Florian Fainelli <f.fainelli@gmail.com
>     <mailto:f.fainelli@gmail.com>> writes:
> 
>     >> In all cases *if caching is really needed*, I think it won't hurt to do
>     >> it in DSA core even if a switch support FDB dump operations on a
>     >> per-port basis, as Andrew mentioned.
>     >
>     > Agreed, and there does not appear to be any need to new dsa_switch_ops
>     > operations to be introduced?
> 
>     Nope.
> 
> 

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Andrew Lunn]

On Fri, Dec 16, 2016 at 10:24:28AM +0100, Volodymyr Bendiuga wrote:
> Hi all,
> 
> Does this mean we all agree on implementing caching
> mechanism in net/dsa layer? If yes, then I can start
> working on it immediately.

Volodymyr

Could you provide us with your test script, which adds multicast MAC
addresses to ports. I would like to do some profiling with it.

	  Thanks
		Andrew

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Andrew Lunn]

On Fri, Dec 16, 2016 at 11:26:01AM +0100, Volodymyr Bendiuga wrote:
> Hi Andrew,
> 
> I don't have any script right now, the code I have is a part of
> the OS, but I could write simple C program which represents
> what we are doing with mc entries and send it to you for profiling.

It would be nice to have a benchmark test we can use to test out
ideas.

Please try to make the code flexible. The slave interface names on my
boards are probably not the same as on your. Also, the number of ports
may differ.

    Thanks
	Andrew

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Volodymyr Bendiuga]

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

Hi Andrew,
Here is the program I promised you.
There is a .c and Makefile attached to this mail. Simply type ”make” to build it.
There is a dependency on libnl3, which needs to be installed. If you don’t have it
Just install it: "apt-get install libnl-3-dev libnl-route-3-dev” if you use ubuntu based
Linux.

What the program does is it creates a libnl cache, inserts into it some hardcoded
Multicast entries, and then adds entries to the kernel (to the bridge and to hardware).
By default the program looks for interfaces that have ”eth” in their names. If your interfaces
Have different names, just correct that line in the code. Otherwise it should just run straight away.
When entries are added the timing is presented. By default the program uses vlan id 1. If you need
To override it, just pass "-v 2” or whatever vlan id you wish.
Please try running the program with hash table patch and without. You should see a significant difference.

Let me know if there is something you need help with.

Thanks,
Volodymyr

[-- Attachment #2: Makefile --]
[-- Type: application/octet-stream, Size: 326 bytes --]


EXEC      = fdbtest
OBJS      = fdbtest.o 
SRCS      = $(OBJS:.o=.c)
DEPS      = $(SRCS:.c=.d)
CFLAGS   += `pkg-config --cflags libnl-route-3.0`
DEPLIBS   = `pkg-config --libs libnl-route-3.0`
LDLIBS   += $(DEPLIBS)

all: Makefile $(EXEC)

$(EXEC): $(OBJS)

clean:
	-@$(RM) $(OBJS) $(EXEC)

distclean: clean
	-@$(RM) $(DEPS)

[-- Attachment #3: fdbtest.c --]
[-- Type: application/octet-stream, Size: 6045 bytes --]

/* 
 * FDB test - insert preprogrammed fdb entries
 * into HW database and measure the time it takes
 * to do the job.
 *
 * Author: Volodymyr Bendiuga <volodymyr.bendiuga@gmail.com>
 */

#include <net/if.h>
#include <netinet/ether.h>
#include <unistd.h>
#include <time.h>

#define _LINUX_IF_H
#include <linux/if_link.h>
#include <netlink/netlink.h>
#include <netlink/route/link.h>
#include <netlink/route/neighbour.h>
#include <netlink/route/link/bridge.h>

struct cb_data {
	int err;
	int vid;
	struct nl_sock *sk;
	void *arg;
};

static void __add(struct nl_object *obj, void *data)
{
	struct cb_data *cbd = data;
	struct nl_sock *sk;
	struct rtnl_neigh *neigh = (struct rtnl_neigh *)obj;

	sk = cbd->sk;
	if (cbd->err)
		return;

        cbd->err = rtnl_neigh_add(sk, neigh, NLM_F_CREATE);
}

static int __insert(struct nl_cache *cache, int ifindex, unsigned char *mac, int vlan)
{
	struct rtnl_neigh *neigh;
	struct nl_addr *addr;
	int err = 0;

        neigh = rtnl_neigh_alloc();
        if (!neigh) {
                printf("Could not allocate netlink neighbour\n");
                return -NLE_NOMEM;
        }

        addr = nl_addr_alloc(ETH_ALEN);
        if (!addr) {
		printf("Could not allocate netlink address\n");
		err = -NLE_NOMEM;
		goto err_put_neigh;
        }

	nl_addr_set_family(addr, AF_LLC);
	nl_addr_set_prefixlen(addr, 48);
        if (nl_addr_set_binary_addr(addr, mac, ETH_ALEN) < 0) {
		printf("Could not set netlink address\n");
		err = -NLE_FAILURE;
		goto err_put_addr;
	}

	rtnl_neigh_set_family(neigh, PF_BRIDGE);
	rtnl_neigh_set_lladdr(neigh, addr);
	rtnl_neigh_set_flags(neigh, NTF_SELF);
	rtnl_neigh_set_state(neigh, NUD_NOARP);
	rtnl_neigh_set_ifindex(neigh, ifindex);
	rtnl_neigh_set_vlan(neigh, vlan);

	err = nl_cache_add(cache, (struct nl_object*)neigh);
	if (err)
		printf("Could not add object to cache, err -> %d", err);

err_put_addr:
	nl_addr_put(addr);
err_put_neigh:
	rtnl_neigh_put(neigh);

	return err;
}

static int __insert_in_vlan(struct nl_cache *cache, char* ifname, unsigned char *mac, int vid)
{
       int ifindex;
       int err = 0;

       ifindex = if_nametoindex(ifname);
       err = __insert(cache, ifindex, mac, vid);

       return err;
}

static int __insert_default_mac(struct nl_cache *cache, char* ifname, int vid)
{
	int err = 0;
	size_t i;
        unsigned char mac[][ETH_ALEN] = {
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x01 }, /* 224.0.0.1 */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x02 }, /* 224.0.0.2 */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x04 }, /* 224.0.0.4   - DVMRP */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x05 }, /* 224.0.0.5   - OSPF */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x06 }, /* 224.0.0.6   - OSPF */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x09 }, /* 224.0.0.9   - RIPv2 */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x0a }, /* 224.0.0.10  - IGRP */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x0c }, /* 224.0.0.12  - DHCP Server/Relay */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x0d }, /* 224.0.0.13  - PIM */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x0e }, /* 224.0.0.14  - RSVP */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x12 }, /* 224.0.0.18  - VRRP*/
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x13 }, /* 224.0.0.19 */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x14 }, /* 224.0.0.20 */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x15 }, /* 224.0.0.21 */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x16 }, /* 224.0.0.22  - IGMP */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x18 }, /* 224.0.0.24  - OSPF */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x66 }, /* 224.0.0.102 - HSRP */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0x6b }, /* 224.0.0.107 - PTP-pdelay */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0xfb }, /* 224.0.0.251 - mDNS */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0xfc }, /* 224.0.0.252 - Link-local multicast DNS */
                { 0x01, 0x00, 0x5e, 0x00, 0x00, 0xfe }  /* 224.0.0.254 */
        };

	for (i = 0; i < sizeof(mac) / sizeof(mac[0]); i++)
		__insert_in_vlan(cache, ifname, mac[i], vid);

	return err;
}

static void __add_fdb_defaults(struct nl_object *obj, void *data)
{
	struct cb_data *cbd = data;
	struct nl_cache *fdb_cache = cbd->arg;
	struct rtnl_link *link = nl_object_priv(obj);
	char *ifname;

	ifname = rtnl_link_get_name(link);
	if (!strncmp(ifname, "eth", 3)) {
		printf("Using interface -> %s\n", ifname);
		cbd->err = __insert_default_mac(fdb_cache, ifname, cbd->vid);
	}
	else
		printf("Skipping interface -> %s\n", ifname);
}

static int usage(int code)
{
	fprintf(stderr, "\nUsage: fdbtest [-h] [-v vid]\n\n"
		"  -h         Show summary of command line options and exit\n"
		"  -v vid     Set vlan id\n"
		"\n");

	return code;
}

int main(int argc , char *argv[])
{
	struct nl_cache *link_cache, *neigh_cache;
	struct nl_sock *nlsk;
	struct cb_data cbd = {.err = 0, .vid = 1 };
	clock_t start, end;
	double time_used;
	int err = 0;
	int c;

	while ((c = getopt(argc, argv, "hv:")) != EOF) {
		switch (c) {
		case 'h':
			return usage(0);

		case 'v':
			cbd.vid = atoi(optarg);
			break;

		default:
			return usage(1);
		}
	}

	printf("Init\n");
	nlsk = nl_socket_alloc();
	if (!nlsk)
		return -NLE_NOMEM;
	err = nl_connect(nlsk, NETLINK_ROUTE);
	if (err)
		goto free_nlsk;

	err = rtnl_link_alloc_cache(nlsk, AF_UNSPEC, &link_cache);
	if (err)
		goto free_nlsk;

	err = rtnl_neigh_alloc_cache(nlsk, &neigh_cache);
	if (err)
		goto free_lcache;

	printf("Using vlan %d\n\n", cbd.vid);
	cbd.arg = neigh_cache;
	nl_cache_foreach(link_cache, __add_fdb_defaults, &cbd);
	err = cbd.err;

	cbd.sk = nlsk;
	start = clock();
	nl_cache_foreach(neigh_cache, __add, &cbd);
	end = clock();
	err = cbd.err;
	time_used = ((double) (end - start)) / CLOCKS_PER_SEC;
	printf("\nTime spent: %f sec\n", time_used);
	
	nl_cache_free(neigh_cache);
	printf("Exit %d\n", err);

 free_lcache:
	nl_cache_free(link_cache);
 free_nlsk:
	nl_socket_free(nlsk);

	return err;
}

[-- Attachment #4: Type: text/plain, Size: 621 bytes --]


> 16 dec. 2016 kl. 12:01 skrev Andrew Lunn <andrew@lunn.ch>:
> 
> On Fri, Dec 16, 2016 at 11:26:01AM +0100, Volodymyr Bendiuga wrote:
>> Hi Andrew,
>> 
>> I don't have any script right now, the code I have is a part of
>> the OS, but I could write simple C program which represents
>> what we are doing with mc entries and send it to you for profiling.
> 
> It would be nice to have a benchmark test we can use to test out
> ideas.
> 
> Please try to make the code flexible. The slave interface names on my
> boards are probably not the same as on your. Also, the number of ports
> may differ.
> 
>    Thanks
> 	Andrew

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Andrew Lunn]

On Fri, Dec 23, 2016 at 10:30:27AM +0100, Volodymyr Bendiuga wrote:
> Hi Andrew,
> Here is the program I promised you.

Hi Volodymyr

Thanks for this. I will try it out beginning of January. I don't have
access to the hardware at the moment.

       Thanks
		Andrew

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[John Crispin]

On 15/12/2016 18:21, Vivien Didelot wrote:
> Hi Volodymyr,
> 
> Volodymyr Bendiuga <volodymyr.bendiuga@gmail.com> writes:
> 
>> Hi Andrew,
>>
>> I have tested the approach you wrote in previous mails, the one
>> with setting next.mac to address we are looking for -1. It seems
>> to be as slow as the original implementation, unfortunately.
> 
> Hum, that is what I was expecting... The ATU GetNext operation
> (alongside an ether_addr_equal() call) should be quite fast.
> 
>> We use 6097 and 6352 chips, and both of them can not do any port
>> filtering in hardware for fdb dump operation. Seems like they would
>> benefit from cache. But I am not sure about other switches.
>>
>> Does anyone know about such feature in other switches?
> 
> Marvell switches cannot filter ATU entries for a specific port, they
> contain a port vector.
> 
> I guess Florian might answer for Broadcom switches, and John might
> answer for Qualcomm switches.
> 
> In all cases *if caching is really needed*, I think it won't hurt to do
> it in DSA core even if a switch support FDB dump operations on a
> per-port basis, as Andrew mentioned.


QCA switches allow defining a port mask in a fdb search/iterate
operation. just had a look in the brcm driver and it filters in software

	John

Re: [PATCH net-next 1/3] net:dsa:mv88e6xxx: use hashtable to store multicast entries

[Vivien Didelot]

Hi Volodymyr,

Volodymyr Bendiuga <volodymyr.bendiuga@gmail.com> writes:

> +struct pvec_tbl_entry {
> +        struct hlist_node entry;
> +        u32 key_crc32; /* key */
> +        u16 pvec;
> +        struct pvec_tbl_key {
> +                u8 addr[ETH_ALEN];
> +                u16 fid;
> +        } key;
> +};
> +
>  struct mv88e6xxx_atu_entry {
>  	u16	fid;
>  	u8	state;

Also, if we were to cache some values in mv88e6xxx, I'd use the existing
structures which match the hardware definition. Easier to understand.

mv88e6xxx_atu_entry already represents an ATU entry with its port
vector, FID, MAC address and more. Do you think it can be used here?

Thanks,

        Vivien