[Intel-wired-lan] [PATCH net-next 10/12] ice: implement static version of ageing

[Drewek, Wojciech]

Hi Olek

Sorry for late response, I didn't manage to answer all your comments before my vacation :)
Will continue this week.

> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin at intel.com>
> Sent: piątek, 21 kwietnia 2023 18:23
> To: Drewek, Wojciech <wojciech.drewek at intel.com>
> Cc: intel-wired-lan at lists.osuosl.org; netdev at vger.kernel.org; Ertman, David M <david.m.ertman at intel.com>;
> michal.swiatkowski at linux.intel.com; marcin.szycik at linux.intel.com; Chmielewski, Pawel <pawel.chmielewski at intel.com>;
> Samudrala, Sridhar <sridhar.samudrala at intel.com>
> Subject: Re: [PATCH net-next 10/12] ice: implement static version of ageing
> 
> From: Wojciech Drewek <wojciech.drewek at intel.com>
> Date: Mon, 17 Apr 2023 11:34:10 +0200
> 
> > From: Michal Swiatkowski <michal.swiatkowski at linux.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 at linux.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).

Makes sense

> 
> > +
> >  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);
> 	}

sure

> 
> 
> > +	}
> > +	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

ok

> 
> > +
> > +	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