Message-ID: <MW4PR11MB5776C243E4904E9A15E014A6FD639@MW4PR11MB5776.namprd11.prod.outlook.com>
Date:   Thu, 20 Apr 2023 10:46:31 +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 04/12] ice: Implement basic eswitch bridge setup



> -----Original Message-----
> From: Drewek, Wojciech
> Sent: czwartek, 20 kwietnia 2023 11:54
> To: Lobakin, Aleksander <aleksander.lobakin@...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 04/12] ice: Implement basic eswitch bridge setup
> 
> Thanks for review Olek!
> 
> Most of the comments sound reasonable to me (and I will include them) with some exceptions.
> 
> 
> > -----Original Message-----
> > From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> > Sent: środa, 19 kwietnia 2023 17:24
> > 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 04/12] ice: Implement basic eswitch bridge setup
> >
> > From: Wojciech Drewek <wojciech.drewek@...el.com>
> > Date: Mon, 17 Apr 2023 11:34:04 +0200
> >
> > > With this patch, ice driver is able to track if the port
> > > representors or uplink port were added to the linux bridge in
> > > switchdev mode. Listen for NETDEV_CHANGEUPPER events in order to
> > > detect this. ice_esw_br data structure reflects the linux bridge
> > > and stores all the ports of the bridge (ice_esw_br_port) in
> > > xarray, it's created when the first port is added to the bridge and
> > > freed once the last port is removed. Note that only one bridge is
> > > supported per eswitch.
> >
> > [...]
> >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> > > index ac2971073fdd..5b2ade5908e8 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > > @@ -511,6 +511,7 @@ struct ice_switchdev_info {
> > >  	struct ice_vsi *control_vsi;
> > >  	struct ice_vsi *uplink_vsi;
> > >  	bool is_running;
> > > +	struct ice_esw_br_offloads *br_offloads;
> >
> > 7-byte hole here unfortunately =\ After ::is_running. You can place
> > ::br_offloads *before* ::is_running to avoid this (well, you'll still
> > have it, but as padding at the end of the structure).
> > ...or change ::is_running to "unsigned long flags" to not waste 1 byte
> > for 1 bit and have 63 free flags more :D
> >
> > >  };
> > >
> > >  struct ice_agg_node {
> >
> > [...]
> >
> > > +static struct ice_esw_br_port *
> > > +ice_eswitch_br_netdev_to_port(struct net_device *dev)
> >
> > Also const?

This function changes a bit in "ice: Accept LAG netdevs in bridge offloads"
With the changes introduced in this commit, I think that @dev as constant is not a good option.

> >
> > > +{
> > > +	if (ice_is_port_repr_netdev(dev)) {
> > > +		struct ice_repr *repr = ice_netdev_to_repr(dev);
> > > +
> > > +		return repr->br_port;
> > > +	} else if (netif_is_ice(dev)) {
> > > +		struct ice_pf *pf = ice_netdev_to_pf(dev);
> >
> > Both @repr and @pf can also be const :p

Repr makes sense to me, the second part will change later and I think that
there is no point in making it const

> >
> > > +
> > > +		return pf->br_port;
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> >
> > [...]
> >
> > > +static struct ice_esw_br_port *
> > > +ice_eswitch_br_port_init(struct ice_esw_br *bridge)
> > > +{
> > > +	struct ice_esw_br_port *br_port;
> > > +
> > > +	br_port = kzalloc(sizeof(*br_port), GFP_KERNEL);
> > > +	if (!br_port)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	br_port->bridge = bridge;
> >
> > Since you always pass @bridge from the call site either way, does it
> > make sense to do that or you could just assign -> bridge on the call
> > sites after a successful allocation?
> 
> I could do that but I prefer to keep it this way.
> We have two types of ports and this function is generic, It setups
> things common for both types, including bridge ref.
> Are you ok with it?
> 
> >
> > > +
> > > +	return br_port;
> > > +}
> >
> > [...]
> >
> > > +static int
> > > +ice_eswitch_br_port_changeupper(struct notifier_block *nb, void *ptr)
> > > +{
> > > +	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> > > +	struct netdev_notifier_changeupper_info *info = ptr;
> > > +	struct ice_esw_br_offloads *br_offloads =
> > > +		ice_nb_to_br_offloads(nb, netdev_nb);
> >
> > Maybe assign it outside the declaration block to avoid line wrap?
> >
> > > +	struct netlink_ext_ack *extack;
> > > +	struct net_device *upper;
> > > +
> > > +	if (!ice_eswitch_br_is_dev_valid(dev))
> > > +		return 0;
> > > +
> > > +	upper = info->upper_dev;
> > > +	if (!netif_is_bridge_master(upper))
> > > +		return 0;
> > > +
> > > +	extack = netdev_notifier_info_to_extack(&info->info);
> > > +
> > > +	return info->linking ?
> > > +		ice_eswitch_br_port_link(br_offloads, dev, upper->ifindex,
> > > +					 extack) :
> > > +		ice_eswitch_br_port_unlink(br_offloads, dev, upper->ifindex,
> > > +					   extack);
> >
> > And here do that via `if return else return` to avoid multi-line ternary?
> >
> > > +}
> > > +
> > > +static int
> > > +ice_eswitch_br_port_event(struct notifier_block *nb,
> > > +			  unsigned long event, void *ptr)
> >
> > [...]
> >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> > > new file mode 100644
> > > index 000000000000..53ea29569c36
> > > --- /dev/null
> > > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> > > @@ -0,0 +1,42 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/* Copyright (C) 2023, Intel Corporation. */
> > > +
> > > +#ifndef _ICE_ESWITCH_BR_H_
> > > +#define _ICE_ESWITCH_BR_H_
> > > +
> > > +enum ice_esw_br_port_type {
> > > +	ICE_ESWITCH_BR_UPLINK_PORT = 0,
> > > +	ICE_ESWITCH_BR_VF_REPR_PORT = 1,
> > > +};
> > > +
> > > +struct ice_esw_br_port {
> > > +	struct ice_esw_br *bridge;
> > > +	enum ice_esw_br_port_type type;
> >
> > Also hole :s I'd move it one line below.
> >
> > > +	struct ice_vsi *vsi;
> > > +	u16 vsi_idx;
> > > +};
> > > +
> > > +struct ice_esw_br {
> > > +	struct ice_esw_br_offloads *br_offloads;
> > > +	int ifindex;
> > > +
> > > +	struct xarray ports;
> >
> > (not sure about this one, but potentially there can be a hole between
> >  those two)
> 
> Move ifindex at the end?
> 
> >
> > > +};
> > > +
> > > +struct ice_esw_br_offloads {
> > > +	struct ice_pf *pf;
> > > +	struct ice_esw_br *bridge;
> > > +	struct notifier_block netdev_nb;
> > > +};
> > > +
> > > +#define ice_nb_to_br_offloads(nb, nb_name) \
> > > +	container_of(nb, \
> > > +		     struct ice_esw_br_offloads, \
> > > +		     nb_name)
> >
> > Hmm, you use it only once and only with `netdev_nb` field. Do you plan
> > to add more call sites of this macro? Otherwise you could embed the
> > second argument into the macro itself (mentioned `netdev_nb`) or even
> > just open-code the whole macro in the sole call site.
> 
> I the next patch it is used with different nb_name (switchdev_nb)
> 
> >
> > > +
> > > +void
> > > +ice_eswitch_br_offloads_deinit(struct ice_pf *pf);
> > > +int
> > > +ice_eswitch_br_offloads_init(struct ice_pf *pf);
> > > +
> > > +#endif /* _ICE_ESWITCH_BR_H_ */
> > [...]
> >
> > Thanks,
> > Olek

Powered by blists - more mailing lists