Re: [net-next 05/16] ice: Add support for tunnel offloads

["Nguyen, Anthony L" <anthony.l.nguyen@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6036 bytes --]

On Wed, 2020-03-04 at 18:46 -0800, Jakub Kicinski wrote:
> On Wed,  4 Mar 2020 15:21:25 -0800 Jeff Kirsher wrote:
> > From: Tony Nguyen <anthony.l.nguyen@intel.com>
> > 
> > Create a boost TCAM entry for each tunnel port in order to get a
> > tunnel
> > PTYPE. Update netdev feature flags and implement the appropriate
> > logic to
> > get and set values for hardware offloads.
> > 
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > Signed-off-by: Henry Tieman <henry.w.tieman@intel.com>
> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > +/**
> > + * ice_create_tunnel
> > + * @hw: pointer to the HW structure
> > + * @type: type of tunnel
> > + * @port: port to use for vxlan tunnel
> > + *
> > + * Creates a tunnel
> 
> I was going to comment how useless this kdoc is, then I realized that
> it's not only useless but incorrect - port doesn't have to be vxlan,
> you support geneve..

Will fix these.

> > + */
> > +enum ice_status
> > +ice_create_tunnel(struct ice_hw *hw, enum ice_tunnel_type type,
> > u16 port)
> > +{
> > +	struct ice_boost_tcam_section *sect_rx, *sect_tx;
> > +	enum ice_status status = ICE_ERR_MAX_LIMIT;
> > +	struct ice_buf_build *bld;
> > +	u16 index;
> > +
> > +	if (ice_tunnel_port_in_use(hw, port, NULL))
> > +		return ICE_ERR_ALREADY_EXISTS;
> 
> Could you explain how ref counting of ports works? It's possible to
> have multiple tunnels on the same port. Looks like this is just
> bailing
> without even making a note of the request. So delete will just remove
> the port whenever the first tunnel with this port goes down?

We aren't doing ref counting of ports so your observation is correct. 
Will rework this and add in ref counting so this doesn't occur.  Thanks
for catching this.

> > +	if (!ice_find_free_tunnel_entry(hw, type, &index))
> > +		return ICE_ERR_OUT_OF_RANGE;
> > +
> > +	bld = ice_pkg_buf_alloc(hw);
> > +	if (!bld)
> > +		return ICE_ERR_NO_MEMORY;
> > +
> > +	/* allocate 2 sections, one for Rx parser, one for Tx parser */
> > +	if (ice_pkg_buf_reserve_section(bld, 2))
> > +		goto ice_create_tunnel_err;
> > +
> > +	sect_rx = (struct ice_boost_tcam_section *)
> > +		ice_pkg_buf_alloc_section(bld,
> > ICE_SID_RXPARSER_BOOST_TCAM,
> 
> this function returns void, the extremely ugly casts are unnecessary

Will remove

> > +					  sizeof(*sect_rx));
> > +	if (!sect_rx)
> > +		goto ice_create_tunnel_err;
> > +	sect_rx->count = cpu_to_le16(1);
> > +
> > +	sect_tx = (struct ice_boost_tcam_section *)
> > +		ice_pkg_buf_alloc_section(bld,
> > ICE_SID_TXPARSER_BOOST_TCAM,
> 
> and here

Will remove

> > +					  sizeof(*sect_tx));
> > +	if (!sect_tx)
> > +		goto ice_create_tunnel_err;
> > +	sect_tx->count = cpu_to_le16(1);
> > +
> > +	/* copy original boost entry to update package buffer */
> > +	memcpy(sect_rx->tcam, hw->tnl.tbl[index].boost_entry,
> > +	       sizeof(*sect_rx->tcam));
> > +
> > +	/* over-write the never-match dest port key bits with the
> > encoded port
> > +	 * bits
> > +	 */
> > +	ice_set_key((u8 *)&sect_rx->tcam[0].key, sizeof(sect_rx-
> > >tcam[0].key),
> > +		    (u8 *)&port, NULL, NULL, NULL,
> > +		    offsetof(struct ice_boost_key_value,
> > hv_dst_port_key),
> > +		    sizeof(sect_rx->tcam[0].key.key.hv_dst_port_key));
> > +
> > +	/* exact copy of entry to Tx section entry */
> > +	memcpy(sect_tx->tcam, sect_rx->tcam, sizeof(*sect_tx->tcam));
> > +
> > +	status = ice_update_pkg(hw, ice_pkg_buf(bld), 1);
> > +	if (!status) {
> > +		hw->tnl.tbl[index].port = port;
> > +		hw->tnl.tbl[index].in_use = true;
> > +	}
> > +
> > +ice_create_tunnel_err:
> > +	ice_pkg_buf_free(hw, bld);
> > +
> > +	return status;
> > +}
> > +/**
> > + * ice_udp_tunnel_add - Get notifications about UDP tunnel ports
> > that come up
> > + * @netdev: This physical port's netdev
> > + * @ti: Tunnel endpoint information
> > + */
> > +static void
> > +ice_udp_tunnel_add(struct net_device *netdev, struct
> > udp_tunnel_info *ti)
> > +{
> > +	struct ice_netdev_priv *np = netdev_priv(netdev);
> > +	struct ice_vsi *vsi = np->vsi;
> > +	struct ice_pf *pf = vsi->back;
> > +	enum ice_tunnel_type tnl_type;
> > +	u16 port = ntohs(ti->port);
> > +	enum ice_status status;
> > +
> > +	switch (ti->type) {
> > +	case UDP_TUNNEL_TYPE_VXLAN:
> > +		tnl_type = TNL_VXLAN;
> > +		break;
> > +	case UDP_TUNNEL_TYPE_GENEVE:
> > +		tnl_type = TNL_GENEVE;
> > +		break;
> > +	default:
> > +		netdev_err(netdev, "Unknown tunnel type\n");
> > +		return;
> > +	}
> > +
> > +	status = ice_create_tunnel(&pf->hw, tnl_type, port);
> > +	if (status == ICE_ERR_ALREADY_EXISTS)
> > +		dev_dbg(ice_pf_to_dev(pf), "port %d already exists in
> > UDP tunnels list\n",
> > +			port);
> > +	else if (status == ICE_ERR_OUT_OF_RANGE)
> > +		netdev_err(netdev, "Max tunneled UDP ports reached,
> > port %d not added\n",
> > +			   port);
> 
> error is probably a little much for resource exhaustion since it's
> not
> going to cause any problem other than a slow down?

Correct, just a slow down. A warning then or did you prefer a dbg?

> > +	else if (status)
> > +		netdev_err(netdev, "Error adding UDP tunnel - %d\n",
> > +			   status);
> > +}
> > +
> > +/**
> > + * ice_udp_tunnel_del - Get notifications about UDP tunnel ports
> > that go away
> > + * @netdev: This physical port's netdev
> > + * @ti: Tunnel endpoint information
> > + */
> > +static void
> > +ice_udp_tunnel_del(struct net_device *netdev, struct
> > udp_tunnel_info *ti)
> > +{
> > +	struct ice_netdev_priv *np = netdev_priv(netdev);
> > +	struct ice_vsi *vsi = np->vsi;
> > +	struct ice_pf *pf = vsi->back;
> > +	u16 port = ntohs(ti->port);
> > +	enum ice_status status;
> > +	bool retval;
> > +	u16 index;
> > +
> > +	retval = ice_tunnel_port_in_use(&pf->hw, port, &index);
> 
> nit: index is never used

Will remove this.

Thanks,
Tony

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3277 bytes --]