Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky <leon@kernel.org>]

On Thu, Nov 24, 2022 at 12:07:48PM +0100, Steffen Klassert wrote:
> On Wed, Nov 23, 2022 at 02:53:10PM +0200, Leon Romanovsky wrote:
> > On Wed, Nov 23, 2022 at 11:36:19AM +0200, Leon Romanovsky wrote:
> > > Thanks for an explanation, trying it now.
> > 
> > Something like that?
> 
> Yes :)

Great, will send proper version on Sunday.

> 
> > 
> > The code is untested yet.
> > 
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index 5076f9d7a752..5819023c32ba 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -1115,6 +1115,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >  	rcu_read_lock();
> >  	h = xfrm_dst_hash(net, daddr, saddr, tmpl->reqid, encap_family);
> >  	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h, bydst) {
> > +		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
> > +		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
> 
> Please try to avoid that check for every state in the list.
> Maybe enable this code with a static key if packet offload
> is used?

I assumed that it will be optimized by compiled because "pol->xdo.type ==
XFRM_DEV_OFFLOAD_PACKET)" is constant here. I'll take a look for more fancy
solutions, but I have serious doubts if they give any benefits.

> 
> > +			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> > +				/* HW states are in the head of list, there is no need
> > +				 * to iterate further.
> > +				 */
> > +				break;
> > +
> > +			/* Packet offload: both policy and SA should have same device */
> > +			if (pol->xdo.dev != x->xso.dev)
> > +				continue;
> > +		}
> > +
> >  		if (x->props.family == encap_family &&
> >  		    x->props.reqid == tmpl->reqid &&
> >  		    (mark & x->mark.m) == x->mark.v &&
> > @@ -1132,6 +1145,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >  
> >  	h_wildcard = xfrm_dst_hash(net, daddr, &saddr_wildcard, tmpl->reqid, encap_family);
> >  	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h_wildcard, bydst) {
> > +		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
> > +		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
> > +			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> > +				/* HW states are in the head of list, there is no need
> > +				 * to iterate further.
> > +				 */
> > +				break;
> > +
> > +			/* Packet offload: both policy and SA should have same device */
> > +			if (pol->xdo.dev != x->xso.dev)
> > +				continue;
> > +		}
> > +
> >  		if (x->props.family == encap_family &&
> >  		    x->props.reqid == tmpl->reqid &&
> >  		    (mark & x->mark.m) == x->mark.v &&
> > @@ -1185,6 +1211,17 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >  			goto out;
> >  		}
> >  
> > +		if (pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
> > +			memcpy(&x->xso, &pol->xdo, sizeof(x->xso));
> > +			error = pol->xdo.dev->xfrmdev_ops->xdo_dev_state_add(x);
> > +			if (error) {
> > +				x->km.state = XFRM_STATE_DEAD;
> > +				to_put = x;
> > +				x = NULL;
> > +				goto out;
> > +			}
> > +		}
> 
> I guess that is to handle acquires, right?

Yes

> What is the idea behind that?

In previous patches, I made sure that policy and SA uses same
struct xfrm_dev_offload and set fields exactly the same.

This line sets all properties::
memcpy(&x->xso, &pol->xdo, sizeof(x->xso));

And .xdo_dev_state_add() gets everything pre-configured.

But yes, it will be different in final patch to make sure that
offload_handle is cleared and dev_tracker is valid.

Thanks