Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Lu Baolu <baolu.lu@linux.intel.com>,
        Joerg Roedel <joro@8bytes.org>,
        "Will Deacon" <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        "Jason Gunthorpe" <jgg@ziepe.ca>,
        Jean-Philippe Brucker <jean-philippe@linaro.org>,
        Nicolin Chen <nicolinc@nvidia.com>
CC:     "Liu, Yi L" <yi.l.liu@intel.com>,
        Jacob Pan <jacob.jun.pan@linux.intel.com>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2 08/12] iommu: Prepare for separating SVA and IOPF
Thread-Topic: [PATCH v2 08/12] iommu: Prepare for separating SVA and IOPF
Thread-Index: AQHZwE5hR532wMNk30qvLyiBGXN6vq/YQ9iw
Date:   Thu, 3 Aug 2023 08:16:47 +0000
Message-ID: <BN9PR11MB52769D22490BB09BB25E0C2E8C08A@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20230727054837.147050-1-baolu.lu@linux.intel.com>
 <20230727054837.147050-9-baolu.lu@linux.intel.com>
In-Reply-To: <20230727054837.147050-9-baolu.lu@linux.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?kjRnKbuhn+7uDr+4jl6w/FbTyYS/vm3bm1FTEvXc5RTWhg4KKeNeCtIhDUW3?=
 =?us-ascii?Q?HfVbTDMi+DLL5fuEMigXj0kKVZwsLr3v5twq1y4MJ1r8wpAtQKPMOszK9JIh?=
 =?us-ascii?Q?Wf690l9iusB7CL7l3ZtwotzaXXmZ8LxaUWTzTpp7aA7jwM1xkPztu0MvwED4?=
 =?us-ascii?Q?fcDgWvEVjtlXL9xSAV+fF/uxxGwDQCJ6W8orJRaqJj9fKTiF3+wYyfJR9E76?=
 =?us-ascii?Q?QJosCY0hlCQnXOMludn8Uej1KjBxRfvmw2t10WT/4VSU2QQgIhIlW/mtpPi3?=
 =?us-ascii?Q?F14HbMp7UfF3xBz/po387mtYJ1N5OPoOAhQK9h8+4aB2mMtOeM1+6Tx/m+SD?=
 =?us-ascii?Q?jglAsMLLIHp/ivpUdBZ/F6sfQqcNj5DxH2Wq5JL5IQBkyPSXVqobjBdT9pBM?=
 =?us-ascii?Q?ewHr6OgcCfxaA2L4wMfBacAgA8VMBhAA0nirkoEzRDDq5TAe3LIBbz2Y+vlI?=
 =?us-ascii?Q?rA81cx2TrYl/0JLB6cQY5bU/9aW5Wyqkx71+bLpDDphDcGWgw0wwmKaC9CZQ?=
 =?us-ascii?Q?6zCGwi3cLuBnIQ/7C5u9ZtVIf0X2h24orhHTdl+G36WwWarlbLPeiMQA1Li2?=
 =?us-ascii?Q?3F176JsMD2FcP9aoIiz8kvFiYUsXRXra1GxZA6WwwIcUK2kBgvKFJ9yczcXQ?=
 =?us-ascii?Q?2EVfqHRrP87+ZqBoEBCNDWROUPUfkNkDYXT0KzFpZEqRdwDVBwDZ7W7oxyPy?=
 =?us-ascii?Q?fJb/FCk624vu8O/+0Q7Ty/bL04pVlo/LVfh0IOAvx8atZrTpewW583TxqYsc?=
 =?us-ascii?Q?h9yLG5026RF4Lbd8VgURAZVCv1vA2stWqFrYzVgAPhfURf3fobdl0u0LtiZw?=
 =?us-ascii?Q?UP7iWysQwwYe5gTv0dzYIhL1X9+yrNXvWcogjjgNkqOiVEO8ZT8JROYy4lkP?=
 =?us-ascii?Q?GVmj7XD6UDNA82HohvHqK0ar8ZTbZ9e92df058nTIeuJjlTO9LwhelGgHejY?=
 =?us-ascii?Q?18TtDGYJ1dGe7X5iglowiqP5hV8u68AodWUXzK4qB+lFnKJMqFkyU5N9bpML?=
 =?us-ascii?Q?b8RN0QP4YCti9QsxQLHDAf9Jxh5rRq4pLh2eYfzsw0V0FL1wQUCK+ScFQVnI?=
 =?us-ascii?Q?O70a5drg95E/njaxSf3ftWNFs94uEBi7Uym5VQGYnqw/IyUKX2jZj/iOzARB?=
 =?us-ascii?Q?guh3GjilYbAYC5e6WhAItdIbMkLEnqtfeic2Sq//mnjDzww2xSw6SkMP4QLg?=
 =?us-ascii?Q?Rg4d++4BPF43bl/IL6Aq8mriC4ckpquURRZBjoBCFDdpt9wcBn9C/heI3hmr?=
 =?us-ascii?Q?RyXPFBdXCG3O0L9ZhF2l/IoEp/3BkF/8d3/jpEEzosNhQ8yWxQ/87QnuyS4p?=
 =?us-ascii?Q?q7ZPcCL/nLuO0GHptjtmvd29lwPhLsolI6tOmTK5nsrBwsybX5lEGaR3Z/kg?=
 =?us-ascii?Q?WjPnKPCpuf5F/Ue7PxtIEkTrcEvLvbWCLojsVPjf0nQkppyRgmzUlAtEPujs?=
 =?us-ascii?Q?vVesyfqIrhAB78FlyAIwS5+eT2T1AjpidloJkX1J01SvKh6eGQZ69Cy+J8Xl?=
 =?us-ascii?Q?bYNYohwla82Nxa3GSE86lrP4LZ0PMHhax5n1VqGof5zZGMuXv0LEsTDW3NOa?=
 =?us-ascii?Q?pQxnXZT+NWqIwBqo5OMkFkqz+wdHhaEYKjFganaG?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d0ae5351-1836-4f87-099c-08db93f9fb8f
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Aug 2023 08:16:47.9117
 (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: MjI8D475fXP07p4CSLOaHxqWpH4RZc6eWTbEr6kSxaiGr6jEuBqOm389u4ie8VeE4cK0uiloinzfg4iFKceySg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5226
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, July 27, 2023 1:49 PM
>
> @@ -82,7 +82,7 @@ static void iopf_handler(struct work_struct *work)
>  	if (!domain || !domain->iopf_handler)
>  		status =3D IOMMU_PAGE_RESP_INVALID;
>=20
> -	list_for_each_entry_safe(iopf, next, &group->faults, list) {
> +	list_for_each_entry(iopf, &group->faults, list) {
>  		/*
>  		 * For the moment, errors are sticky: don't handle
> subsequent
>  		 * faults in the group if there is an error.
> @@ -90,14 +90,20 @@ static void iopf_handler(struct work_struct *work)
>  		if (status =3D=3D IOMMU_PAGE_RESP_SUCCESS)
>  			status =3D domain->iopf_handler(&iopf->fault,
>  						      domain->fault_data);
> -
> -		if (!(iopf->fault.prm.flags &
> -		      IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
> -			kfree(iopf);
>  	}
>=20
>  	iopf_complete_group(group->dev, &group->last_fault, status);
> -	kfree(group);
> +	iopf_free_group(group);
> +}

this is perf-critical path. It's not good to traverse the list twice.

> +
> +static int iopf_queue_work(struct iopf_group *group, work_func_t func)
> +{
> +	struct iopf_device_param *iopf_param =3D group->dev->iommu-
> >iopf_param;
> +
> +	INIT_WORK(&group->work, func);
> +	queue_work(iopf_param->queue->wq, &group->work);
> +
> +	return 0;
>  }

Is there plan to introduce further error in the future? otherwise this shou=
ld
be void.

btw the work queue is only for sva. If there is no other caller this can be
just kept in iommu-sva.c. No need to create a helper.

> @@ -199,8 +204,11 @@ int iommu_queue_iopf(struct iommu_fault *fault,
> struct device *dev)
>  			list_move(&iopf->list, &group->faults);
>  	}
>=20
> -	queue_work(iopf_param->queue->wq, &group->work);
> -	return 0;
> +	ret =3D iopf_queue_work(group, iopf_handler);
> +	if (ret)
> +		iopf_free_group(group);
> +
> +	return ret;
>=20

Here we can document that the iopf handler (in patch10) should free the
group, allowing the optimization inside the handler.