Re: [PATCH net-next v2 2/3] nfp: add framework to support ipsec offloading

[Leon Romanovsky <leon@kernel.org>]

On Mon, Oct 10, 2022 at 03:05:12PM +0800, Yinjun Zhang wrote:
> Thanks for your comments and sorry for the late reply.
> 
> On Thu, Sep 29, 2022 at 11:10:12AM +0300, Leon Romanovsky wrote:
> > On Tue, Sep 27, 2022 at 12:27:06PM +0200, Simon Horman wrote:
> > > +struct nfp_net_ipsec_data {
> > > +	struct nfp_net_ipsec_sa_data sa_entries[NFP_NET_IPSEC_MAX_SA_CNT];
> > > +	unsigned int sa_free_stack[NFP_NET_IPSEC_MAX_SA_CNT];
> > > +	unsigned int sa_free_cnt;
> > 
> > I don't see in this patch what are you doing with this free_stack array,
> > but whole nfp_net_ipsec_data is more than 32Kb of arrays.
> >
> 
> `sa_free_stack` is used to maintain the used/available sa entries, which
> is initialized in `nfp_net_ipsec_init`.
> Yes, it's indeed a big array, and we're going to use pointer instead of array
> here.

Why do you want to use array and not Xarray?

> 
> > > +bool nfp_net_ipsec_tx_prep(struct nfp_net_dp *dp, struct sk_buff *skb,
> > > +			   struct nfp_ipsec_offload *offload_info)
> > > +{
> > > +	struct xfrm_offload *xo = xfrm_offload(skb);
> > > +	struct xfrm_state *x;
> > > +
> > > +	if (!xo)
> > > +		return false;
> > 
> > How is it possible in offload path?
> > Why do all drivers check sec_path length and not xo?
> > 
> 
> `tx_prep` is called in the tx datapath, we use `xo` to check if the
> packet needs offload-encrypto or not.

You didn't answer on any of my questions above.

How is it possible in offload path?
Why do all drivers check sec_path length and not xo?

> 
> > > +int nfp_net_ipsec_rx(struct nfp_meta_parsed *meta, struct sk_buff *skb)
> > > +{
> > > +	struct nfp_net_ipsec_sa_data *sa_data;
> > > +	struct net_device *netdev = skb->dev;
> > > +	struct nfp_net_ipsec_data *ipd;
> > > +	struct xfrm_offload *xo;
> > > +	struct nfp_net_dp *dp;
> > > +	struct xfrm_state *x;
> > > +	struct sec_path *sp;
> > > +	struct nfp_net *nn;
> > > +	int saidx;
> > > +
> > > +	nn = netdev_priv(netdev);
> > > +	ipd = nn->ipsec_data;
> > > +	dp = &nn->dp;
> > > +
> > > +	if (meta->ipsec_saidx == 0)
> > > +		return 0; /* No offload took place */
> > > +
> > > +	saidx = meta->ipsec_saidx - 1;
> > > +	if (saidx > NFP_NET_IPSEC_MAX_SA_CNT || saidx < 0) {
> > > +		nn_dp_warn(dp, "Invalid SAIDX from NIC %d\n", saidx);
> > 
> > No prints in data path that can be triggered from the network, please.
> > 
> 
> It's a ratelimit print, and it means severe error happens, probably
> unrecoverable, when running into this path.

The main part of the sentence is "... can be triggered from the network ..."

Thanks