netdev - Re: [PATCH net-next 10/12] ice: implement static version of ageing

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: <a868e53c-add1-986e-7c96-a02afbddde1e@intel.com>
Date:   Fri, 21 Apr 2023 18:22:46 +0200
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Wojciech Drewek <wojciech.drewek@...el.com>
CC:     <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
        <david.m.ertman@...el.com>, <michal.swiatkowski@...ux.intel.com>,
        <marcin.szycik@...ux.intel.com>, <pawel.chmielewski@...el.com>,
        <sridhar.samudrala@...el.com>
Subject: Re: [PATCH net-next 10/12] ice: implement static version of ageing

From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Mon, 17 Apr 2023 11:34:10 +0200

> From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> 
> Remove fdb entries always when ageing time expired.
> 
> Allow user to set ageing time using port object attribute.
> 
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> ---
>  .../net/ethernet/intel/ice/ice_eswitch_br.c   | 46 +++++++++++++++++++
>  .../net/ethernet/intel/ice/ice_eswitch_br.h   | 11 +++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> index a21eca5088f7..6c3144f98100 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> @@ -8,6 +8,8 @@
>  #include "ice_vlan.h"
>  #include "ice_vf_vsi_vlan_ops.h"
>  
> +#define ICE_ESW_BRIDGE_UPDATE_INTERVAL_MS 1000

I think you can define it without '_MS' and as msecs_to_jiffies(1000)
right here, so that you wouldn't need to convert it at use sites (it's
more expensive to do there in terms of chars vs line width).

> +
>  static const struct rhashtable_params ice_fdb_ht_params = {
>  	.key_offset = offsetof(struct ice_esw_br_fdb_entry, data),
>  	.key_len = sizeof(struct ice_esw_br_fdb_data),
> @@ -440,6 +442,7 @@ ice_eswitch_br_fdb_entry_create(struct net_device *netdev,
>  	fdb_entry->br_port = br_port;
>  	fdb_entry->flow = flow;
>  	fdb_entry->dev = netdev;
> +	fdb_entry->last_use = jiffies;
>  	event = SWITCHDEV_FDB_ADD_TO_BRIDGE;
>  
>  	if (added_by_user) {
> @@ -838,6 +841,10 @@ ice_eswitch_br_port_obj_attr_set(struct net_device *netdev, const void *ctx,
>  		ice_eswitch_br_vlan_filtering_set(br_port->bridge,
>  						  attr->u.vlan_filtering);
>  		break;
> +	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
> +		br_port->bridge->ageing_time =
> +			clock_t_to_jiffies(attr->u.ageing_time);

Why reviews also teach the reviewer himself -- because I never knew of
clock_t and that userspace has its own ticks, which we have to convert O_.

(sounds as a joke BTW, why not just use ms/us/ns everywhere, "tick" is
 something very intimate/internal)

> +		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}

[...]

> +	if (!bridge)
> +		return;
> +
> +	rtnl_lock();
> +	list_for_each_entry_safe(entry, tmp, &bridge->fdb_list, list) {
> +		if (entry->flags & ICE_ESWITCH_BR_FDB_ADDED_BY_USER)
> +			continue;
> +
> +		if (time_is_before_jiffies(entry->last_use +
> +					   bridge->ageing_time))
> +			ice_eswitch_br_fdb_entry_notify_and_cleanup(bridge,
> +								    entry);

Maybe invert the condition to give a bit more space for arguments?

		if (time_is_after_eq_jiffies(entry->last_use +
					     bridge->ageing_time))
			continue;

		ice_eswitch_br_fdb_entry_notify_and_cleanup(bridge, entry);
	}


> +	}
> +	rtnl_unlock();
> +}
> +
> +static void ice_eswitch_br_update_work(struct work_struct *work)
> +{
> +	struct ice_esw_br_offloads *br_offloads =
> +		ice_work_to_br_offloads(work);

Assign it in a separate line pls :s

> +
> +	ice_eswitch_br_update(br_offloads);
> +
> +	queue_delayed_work(br_offloads->wq, &br_offloads->update_work,
> +			   msecs_to_jiffies(ICE_ESW_BRIDGE_UPDATE_INTERVAL_MS));
> +}
[...]

Thanks,
Olek