[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Fri, 25 Nov 2022 08:23:47 +0200
From: Leon Romanovsky <leon@...nel.org>
To: Steffen Klassert <steffen.klassert@...unet.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Herbert Xu <herbert@...dor.apana.org.au>,
Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies
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
Powered by blists - more mailing lists