netdev - Re: [PATCH net-next v2 3/7] ibmvnic: avoid allocating rwi entries

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: <ee2fe19334bc8a23009df4cf1c54731bacb7d95c.camel@kernel.org>
Date:   Tue, 12 Jan 2021 11:48:58 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Sukadev Bhattiprolu <sukadev@...ux.ibm.com>, netdev@...r.kernel.org
Cc:     Dany Madden <drt@...ux.ibm.com>, Lijun Pan <ljp@...ux.ibm.com>,
        Rick Lindsley <ricklind@...ux.ibm.com>
Subject: Re: [PATCH net-next v2 3/7] ibmvnic: avoid allocating rwi entries

On Tue, 2021-01-12 at 10:14 -0800, Sukadev Bhattiprolu wrote:
> Whenever we need to schedule a reset, we allocate an rwi (reset work
> item?) entry and add to the list of pending resets.
> 
> Since we only add one rwi for a given reason type to the list (no
> duplicates).
> we will only have a handful of reset types in the list - even in the
> worst case. In the common case we should just have a couple of
> entries
> at most.
> 
> Rather than allocating/freeing every time (and dealing with the
> corner
> case of the allocation failing), use a fixed number of rwi entries.
> The extra memory needed is tiny and most of it will be used over the
> active life of the adapter.
> 
> This also fixes a couple of tiny memory leaks. One is in
> ibmvnic_reset()
> where we don't free the rwi entries after deleting them from the list
> due
> to a transport event.  The second is in __ibmvnic_reset() where if we
> find that the adapter is being removed, we simply break out of the
> loop
> (with rc = EBUSY) but ignore any rwi entries that remain on the list.
> 
> Fixes: 2770a7984db58 ("Introduce hard reset recovery")
> Fixes: 36f1031c51a2 ("ibmvnic: Do not process reset during or after
> device removal")
> Signed-off-by: Sukadev Bhattiprolu <sukadev@...ux.ibm.com>
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 123 +++++++++++++++++--------
> ----
>  drivers/net/ethernet/ibm/ibmvnic.h |  14 ++--
>  2 files changed, 78 insertions(+), 59 deletions(-)
> 

[...]

> -enum ibmvnic_reset_reason {VNIC_RESET_FAILOVER = 1,
> +enum ibmvnic_reset_reason {VNIC_RESET_UNUSED = 0,
> +			   VNIC_RESET_FAILOVER = 1,
>  			   VNIC_RESET_MOBILITY,
>  			   VNIC_RESET_FATAL,
>  			   VNIC_RESET_NON_FATAL,
>  			   VNIC_RESET_TIMEOUT,
> -			   VNIC_RESET_CHANGE_PARAM};
> -
> -struct ibmvnic_rwi {
> -	enum ibmvnic_reset_reason reset_reason;
> -	struct list_head list;
> -};
> +			   VNIC_RESET_CHANGE_PARAM,
> +			   VNIC_RESET_MAX};	// must be last
       this is not the preferred comment style: ^^

I would just drop the comment here, it is clear from the name of the
enum.