netdev - RE: [PATCH net-next 2/2] bnx2x: allocate mac filtering pending list in PAGE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BL2PR07MB2306CEA6AD75D9D4EB77FB1F8DF50@BL2PR07MB2306.namprd07.prod.outlook.com>
Date:   Sun, 18 Sep 2016 10:25:05 +0000
From:   "Mintz, Yuval" <Yuval.Mintz@...ium.com>
To:     Jason Baron <jbaron@...mai.com>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Ariel.Elior@...gic.com" <Ariel.Elior@...gic.com>
Subject: RE: [PATCH net-next 2/2] bnx2x: allocate mac filtering pending list
 in PAGE_SIZE increments

> Currently, we can have high order page allocations that specify
> GFP_ATOMIC when configuring multicast MAC address filters.
> 
> For example, we have seen order 2 page allocation failures with
> ~500 multicast addresses configured.
> 
> Convert the allocation for the pending list to be done in PAGE_SIZE
> increments.
> 
> Signed-off-by: Jason Baron <jbaron@...mai.com>

While I appreciate the effort, I wonder whether it's worth it:

- The hardware [even in its newer generation] provides an approximate
based classification [I.e., hashed] with 256 bins.
When configuring 500 multicast addresses, one can argue the
difference between multicast-promisc mode and actual configuration
is insignificant.
Perhaps the easier-to-maintain alternative would simply be to
determine the maximal number of multicast addresses that can be
configured using a single PAGE, and if in need of more than that
simply move into multicast-promisc.

 - While GFP_ATOMIC is required in this flow due to the fact it's being
called from sleepless context, I do believe this is mostly a remnant -
it's possible that by slightly changing the locking scheme we can have
the configuration done from sleepless context and simply switch to
GFP_KERNEL instead.

Regarding the patch itself, only comment I have:
> +			elem_group = (struct bnx2x_mcast_elem_group *)
> +				     elem_group->mcast_group_link.next;
Let's use list_next_entry() instead.