Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Fri, 20 Oct 2023 12:34:54 +0100
From:   Giovanni Cabiddu <giovanni.cabiddu@intel.com>
To:     Mikulas Patocka <mpatocka@redhat.com>
CC:     Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        Michael Hrivnak <mhrivnak@redhat.com>,
        Eric Garver <egarver@redhat.com>, <qat-linux@intel.com>,
        <linux-crypto@vger.kernel.org>, <dm-devel@redhat.com>
Subject: Re: [PATCH v2] qat: fix deadlock in backlog processing
Message-ID: <ZTJl3lgVujG7NCll@gcabiddu-mobl1.ger.corp.intel.com>
References: <af9581e2-58f9-cc19-428f-6f18f1f83d54@redhat.com>
 <ZQ2vJNs/7ZzY44z1@gcabiddu-mobl1.ger.corp.intel.com>
 <ed935382-98ee-6f5d-2f-7c6badfd3abb@redhat.com>
 <ZRc75XGd92MgaVko@gcabiddu-mobl1.ger.corp.intel.com>
 <6b5cb6f-e4bb-8328-a718-f21b2575b8@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6b5cb6f-e4bb-8328-a718-f21b2575b8@redhat.com>
Organization: Intel Research and Development Ireland Ltd - Co. Reg. #308263 -
 Collinstown Industrial Park, Leixlip, County Kildare - Ireland
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?AeS+KGM3O/uabj1Oav13fbJ+lb402mzQmOkERd+EPA4VtHZk/y0saRE/Id66?=
 =?us-ascii?Q?2Y/FkK51NHJUfOI6hTT+4KL/B5hovTipEolTpDKXXd+qoZThFL6WoNr0urnf?=
 =?us-ascii?Q?Who9LON2yECVARq4Ef4Rne5fR8FfJlTl/yk2+ZPNVghRx1LSmj5jmdO213ED?=
 =?us-ascii?Q?lwKK8QIQeTnsUhZOjok0WGyibWIerf3y0K4EdhJ9tER4LR07RNBnxQPKOxx+?=
 =?us-ascii?Q?lydl10mHkv3uFOKkst+atek+zYmuHFAGm+dA1Opf1DyDZLIA5C2NY4v6iNe6?=
 =?us-ascii?Q?jng2ToddVCvoQFD9n8+kznEjOjzYYv9+9fmLUTxj3gDkjSij4Uk+G0q8XqNC?=
 =?us-ascii?Q?t4CkAtIMXPCe38QKw7QPhy6YXgercd1G1f0Y7zUNzblyUuckYzdPMm0G/+mV?=
 =?us-ascii?Q?xseaxkF9AyNDfD04DjQyiDRPoKOkzIA0tJGRAE0THiyNu28rcfqySjqm63gS?=
 =?us-ascii?Q?SzqLD82NXgwoIp8BLe0g3kEXigdhJ3xP4yagmcOIXcHAAlswGOv97+w4teHX?=
 =?us-ascii?Q?uI8lL+6mASD8nCnbx92Yv2AqUsNO64rbX5y2JJtw/umCiz8WEw56TRtcTLut?=
 =?us-ascii?Q?Y8S/nnhBAXnX7XT/bUrYM41B+B+nErJJl+ygcILNjkAuxcBfU4wAqSZOegc3?=
 =?us-ascii?Q?CFG3GTLfeWxqWbKLb1hrd0vtdBaaa4iFNka8O+xSUbiDejXJnr18H87PSe5+?=
 =?us-ascii?Q?VTW0vaQQU5RNhNe+V3mDTptOdwiVxhAPHYA3XvefHJDzbtuM6aLF7SQTiDwi?=
 =?us-ascii?Q?7lZYtQ9rzMGhsj2iNy0DZUH9qKe0et/AgSqTLTIz1PU/c17fkecA3naTNAfE?=
 =?us-ascii?Q?xrr8Y///bmKZpPfYm3sPjBedzBbtDr2FVgANSLkLTIF1V8/4//2Hi6dXjlwo?=
 =?us-ascii?Q?32WOZOJY4fd0LjiBX75FWw2LBhlt+maIYMTAz7FSJ3lhlEcolN2tA8vVdgLv?=
 =?us-ascii?Q?CF9AjNMqPF3d0kfw4Du8Oe+NNtsu9KNmHPOH2pIcZnVw7/w26ewyueLVutDw?=
 =?us-ascii?Q?mZaJpc2bflcS8QXIzirwDX3hjOY89Ucmvalg1ZNMhjN9fB/nUORohuX0cyjo?=
 =?us-ascii?Q?5HsKR/VWl3dfjf29T3eORH676wXL3w8+UqGRQMHaeEmIh3xYzooIZEg4MWwD?=
 =?us-ascii?Q?bJa1uGkWWp3gH0nnYf9hlTj8591AHRQTwxLNVdfUXLarWyo8j6ipA/GOE05S?=
 =?us-ascii?Q?jyyXNnzrEGVQrTphTxrMGTz1rOEAkJSSAIFOYTD+XwZrQaHyJ2HQk0WWopu7?=
 =?us-ascii?Q?dPlJII6YjJd6TtQoRxUyiOmjJ0/TGsWNXotS7mGBUdUmM8kUp0yusSn/znYQ?=
 =?us-ascii?Q?ldm517UARKDjF2MjcUeo3n5xLhpDlWN41/KC16FZrny2dqnK0f5Fhc+dDWeJ?=
 =?us-ascii?Q?D2DNf0RMLHBB8C3AjQV29oaVnhp/DqVEVLsoNLFQ5Ao7Qkyu3msxtMWcTAr1?=
 =?us-ascii?Q?SFPPBSv91P8oqZdI/Vr/RBYOAh4V47PNsFPG7tbfHM4hpJeI6PhAEQy4OFtf?=
 =?us-ascii?Q?WiZuQRM+a0KS0N/bOsTN2w370ofWy6p2BbzVizUBoO76pDd4bkRb9ZCa74O+?=
 =?us-ascii?Q?KpvD3DyfjJ2L/Yg9iph1DDWOBZAETmCVfULKNfLi0t09pnbvFVquDnYJfz5w?=
 =?us-ascii?Q?cw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8b698bc0-c1d0-4bb4-5b81-08dbd1609729
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6366.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Oct 2023 11:34:58.8877
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PboI6xoI2cVdBLdEtQISZKTvtNU6BLzkyL1Aw38HHKJMkqzgtml8ApHirKEwladOQe3KRFSHf2fGOZkU8EwuZZSCmF8PKUzg9yZ9vxYCRkk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB6945
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Fri, 20 Oct 2023 04:35:10 -0700 (PDT)

On Mon, Oct 02, 2023 at 11:15:05AM +0200, Mikulas Patocka wrote:
> 
> 
> On Fri, 29 Sep 2023, Giovanni Cabiddu wrote:
> 
> > On Fri, Sep 22, 2023 at 06:34:16PM +0200, Mikulas Patocka wrote:
> > >
> > > > Also, deserves a fixes tag.
> > > 
> > > "Fixes" tag is for something that worked and that was broken in some 
> > > previous commit.
> > That's right.
> > 
> > > A quick search through git shows that QAT backlogging was 
> > > broken since the introduction of QAT.
> > The driver was moved from drivers/crypto/qat to drivers/crypto/intel/qat
> > that's why you see a single patch.
> > This fixes 386823839732 ("crypto: qat - add backlog mechanism")
> 
> But before 386823839732 it also didn't work - it returned -EBUSY without 
> queuing the request and deadlocked.
> 
> > Thanks - when I proposed the rework I was thinking at a solution without
> > gotos. Here is a draft:
> 
> Yes - it is possible to fix it this way.
> 
> 
> 
> > ------------8<----------------
> >  .../intel/qat/qat_common/qat_algs_send.c      | 40 ++++++++++---------
> >  1 file changed, 22 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/crypto/intel/qat/qat_common/qat_algs_send.c b/drivers/crypto/intel/qat/qat_common/qat_algs_send.c
> > index bb80455b3e81..18c6a233ab96 100644
> > --- a/drivers/crypto/intel/qat/qat_common/qat_algs_send.c
> > +++ b/drivers/crypto/intel/qat/qat_common/qat_algs_send.c
> > @@ -40,17 +40,7 @@ void qat_alg_send_backlog(struct qat_instance_backlog *backlog)
> >  	spin_unlock_bh(&backlog->lock);
> >  }
> >  
> > -static void qat_alg_backlog_req(struct qat_alg_req *req,
> > -				struct qat_instance_backlog *backlog)
> > -{
> > -	INIT_LIST_HEAD(&req->list);
> > -
> > -	spin_lock_bh(&backlog->lock);
> > -	list_add_tail(&req->list, &backlog->list);
> > -	spin_unlock_bh(&backlog->lock);
> > -}
> > -
> > -static int qat_alg_send_message_maybacklog(struct qat_alg_req *req)
> > +static bool qat_alg_try_enqueue(struct qat_alg_req *req)
> >  {
> >  	struct qat_instance_backlog *backlog = req->backlog;
> >  	struct adf_etr_ring_data *tx_ring = req->tx_ring;
> > @@ -58,22 +48,36 @@ static int qat_alg_send_message_maybacklog(struct qat_alg_req *req)
> >  
> >  	/* If any request is already backlogged, then add to backlog list */
> >  	if (!list_empty(&backlog->list))
> > -		goto enqueue;
> > +		return false;
> >  
> >  	/* If ring is nearly full, then add to backlog list */
> >  	if (adf_ring_nearly_full(tx_ring))
> > -		goto enqueue;
> > +		return false;
> >  
> >  	/* If adding request to HW ring fails, then add to backlog list */
> >  	if (adf_send_message(tx_ring, fw_req))
> > -		goto enqueue;
> > +		return false;
> >  
> > -	return -EINPROGRESS;
> > +	return true;
> > +}
> >  
> > -enqueue:
> > -	qat_alg_backlog_req(req, backlog);
> >  
> > -	return -EBUSY;
> > +static int qat_alg_send_message_maybacklog(struct qat_alg_req *req)
> > +{
> > +	struct qat_instance_backlog *backlog = req->backlog;
> > +	int ret = -EINPROGRESS;
> > +
> > +	if (qat_alg_try_enqueue(req))
> > +		return ret;
> > +
> > +	spin_lock_bh(&backlog->lock);
> > +	if (!qat_alg_try_enqueue(req)) {
> > +		list_add_tail(&req->list, &backlog->list);
> > +		ret = -EBUSY;
> > +	}
> > +	spin_unlock_bh(&backlog->lock);
> > +
> > +	return ret;
> >  }
> >  
> >  int qat_alg_send_message(struct qat_alg_req *req)
> > -- 
> > 2.41.0
> 
> 
> Reviwed-by: Mikulas Patocka <mpatocka@redhat.com>
Thanks. I'm going to resend it with a slight change on the comments in
the function qat_alg_try_enqueue().

Regards,

-- 
Giovanni