[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <MW4PR11MB57768C748EAECE1F76B53975FD769@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Tue, 9 May 2023 10:55:53 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: "Lobakin, Aleksander" <aleksander.lobakin@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Ertman, David M"
<david.m.ertman@...el.com>, "michal.swiatkowski@...ux.intel.com"
<michal.swiatkowski@...ux.intel.com>, "marcin.szycik@...ux.intel.com"
<marcin.szycik@...ux.intel.com>, "Chmielewski, Pawel"
<pawel.chmielewski@...el.com>, "Samudrala, Sridhar"
<sridhar.samudrala@...el.com>
Subject: RE: [PATCH net-next 10/12] ice: implement static version of ageing
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@...el.com>
> Sent: piÄ…tek, 21 kwietnia 2023 18:23
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Ertman, David M <david.m.ertman@...el.com>;
> michal.swiatkowski@...ux.intel.com; marcin.szycik@...ux.intel.com; Chmielewski, Pawel <pawel.chmielewski@...el.com>;
> Samudrala, Sridhar <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).
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
Powered by blists - more mailing lists