From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2C16BC4332F for ; Fri, 22 Oct 2021 15:29:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0287760F0F for ; Fri, 22 Oct 2021 15:29:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233321AbhJVPcH (ORCPT ); Fri, 22 Oct 2021 11:32:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40074 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233328AbhJVPcD (ORCPT ); Fri, 22 Oct 2021 11:32:03 -0400 Received: from mail-oi1-x233.google.com (mail-oi1-x233.google.com [IPv6:2607:f8b0:4864:20::233]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2559CC061767 for ; Fri, 22 Oct 2021 08:29:46 -0700 (PDT) Received: by mail-oi1-x233.google.com with SMTP id s9so5471897oiw.6 for ; Fri, 22 Oct 2021 08:29:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel-dk.20210112.gappssmtp.com; s=20210112; h=subject:from:to:cc:references:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=LUtnq1wX3c9ZSKlFVqBQ9milrt9oUJPe3W7pJqk8fis=; b=Dt6Xe7RT9/rmETzyvm6KPZXudZiAHUA2T8XHt//MeIXHgfevOpKLZ+t5VvL4srw/Ba IQEzzktExKHA9fZfICrEpqT1kmakmZoRxQFEv+4IFM6Fopm+2rwRHhgAhC0W8wWc83rS oOh71WpSfXuHTo8oZnZGplwUy/o5P257gkK9AzYegblq//FMB6WsRJrP8apwa+7GnNP1 10vtC0aguxaaRjQXa4kABPrDsqWyZa1BUPPIFw/DIKZ/uDIv8Vgz1rr1hu4xisGj2gcr 7Y6f7k4EWgGMsoC0QmpSDGb6rzpmYI02gymq27osbsFKaiB0vsyY3jt2dt662L+3wHPu k3jg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:from:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=LUtnq1wX3c9ZSKlFVqBQ9milrt9oUJPe3W7pJqk8fis=; b=gTt0PreH0C2+BnIGEAAUdwDxLflu9RpX7tPtWJHeeO72xYZaADR6r3XGv0Q9qY9WSf Nf5TM7SJ/0TRbDAHMubsOxQy1mqB/haxqnyUrYQK67jRXJUV77UBeo09MO3sEvGVOfiS AWKzC8vZuoIkgC3htO4bl7lIA3Hd1m//QyNrOQU93x14PukD02+m4z3iISH0SGNyr5Ec HYPXLIeHddh+Q8VcH1s08CVvqeVkrAd0btXXiFyNIn33QHGgLR07oGWqwfNwwbCd/9W1 FrO6s0dHIf+8Icrc5uOO5Ylq0RVLv6uDm/PQhHCAmdIvdyN4b7UvBM0bx/+3IbbM6fhJ igSg== X-Gm-Message-State: AOAM5324FctLgluIInSA0vZXv3e8zIFtE8fqeLSPe67A4iD5RfeNhP5F z/i2dMqs3NeG+B5CleSvULioDDPFSS0VFA== X-Google-Smtp-Source: ABdhPJx1jjUdC1bsdaA7We5wTASWQ/NVsI6ael3sh50HPmv4zQ9u/R/KB+jBRoiRr7junic+9/3XsQ== X-Received: by 2002:aca:ac0b:: with SMTP id v11mr10588962oie.155.1634916585185; Fri, 22 Oct 2021 08:29:45 -0700 (PDT) Received: from [172.20.15.86] (rrcs-24-173-18-66.sw.biz.rr.com. [24.173.18.66]) by smtp.gmail.com with ESMTPSA id v22sm1491435oot.43.2021.10.22.08.29.44 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 22 Oct 2021 08:29:44 -0700 (PDT) Subject: Re: [PATCH v2] fs: replace the ki_complete two integer arguments with a single argument From: Jens Axboe To: Jeff Moyer Cc: Christoph Hellwig , "linux-fsdevel@vger.kernel.org" , "linux-block@vger.kernel.org" , linux-aio@kvack.org, linux-usb@vger.kernel.org References: <4d409f23-2235-9fa6-4028-4d6c8ed749f8@kernel.dk> <4d3c5a73-889c-2e2c-9bb2-9572acdd11b7@kernel.dk> <6338ba2b-cd71-f66d-d596-629c2812c332@kernel.dk> <7a697483-8e44-6dc3-361e-ae7b62b82074@kernel.dk> Message-ID: <67127b02-2b58-5944-8bfb-e842182d6459@kernel.dk> Date: Fri, 22 Oct 2021 09:29:43 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org On 10/22/21 8:19 AM, Jens Axboe wrote: > On 10/21/21 3:03 PM, Jeff Moyer wrote: >> Jeff Moyer writes: >> >>> Jens Axboe writes: >>> >>>> On 10/21/21 12:05 PM, Jeff Moyer wrote: >>>>> >>>>>>> I'll follow up if there are issues. >>>>> >>>>> s390 (big endian, 64 bit) is failing libaio test 21: >>>>> >>>>> # harness/cases/21.p >>>>> Expected -EAGAIN, got 4294967285 >>>>> >>>>> If I print out both res and res2 using %lx, you'll see what happened: >>>>> >>>>> Expected -EAGAIN, got fffffff5,ffffffff >>>>> >>>>> The sign extension is being split up. >>>> >>>> Funky, does it work if you apply this on top? >>>> >>>> diff --git a/fs/aio.c b/fs/aio.c >>>> index 3674abc43788..c56437908339 100644 >>>> --- a/fs/aio.c >>>> +++ b/fs/aio.c >>>> @@ -1442,8 +1442,8 @@ static void aio_complete_rw(struct kiocb *kiocb, u64 res) >>>> * 32-bits of value at most for either value, bundle these up and >>>> * pass them in one u64 value. >>>> */ >>>> - iocb->ki_res.res = lower_32_bits(res); >>>> - iocb->ki_res.res2 = upper_32_bits(res); >>>> + iocb->ki_res.res = (long) (res & 0xffffffff); >>>> + iocb->ki_res.res2 = (long) (res >> 32); >>>> iocb_put(iocb); >>>> } >>> >>> I think you'll also need to clamp any ki_complete() call sites to 32 >>> bits (cast to int, or what have you). Otherwise that sign extension >>> will spill over into res2. >>> >>> fwiw, I tested with this: >>> >>> iocb->ki_res.res = (long)(int)lower_32_bits(res); >>> iocb->ki_res.res2 = (long)(int)upper_32_bits(res); >>> >>> Coupled with the call site changes, that made things work for me. >> >> This is all starting to feel like a minefield. If you don't have any >> concrete numbers to show that there is a speedup, I think we should >> shelf this change. > > It's really not a minefield at all, we just need a proper help to encode > the value. I'm out until Tuesday, but I'll sort it out when I get back. > Can also provide some numbers on this. I think this incremental should fix it, also providing a helper to properly pack these. The more I look at the gadget stuff the more I also get the feeling that it really is wonky and nobody uses res2, which would be a nice cleanup to continue. But I think it should be separate. diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 8536f19d3c9a..9c5372229714 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -831,7 +831,7 @@ static void ffs_user_copy_worker(struct work_struct *work) kthread_unuse_mm(io_data->mm); } - io_data->kiocb->ki_complete(io_data->kiocb, ((u64) ret << 32) | ret); + io_data->kiocb->ki_complete(io_data->kiocb, aio_res_pack(ret, ret)); if (io_data->ffs->ffs_eventfd && !kiocb_has_eventfd) eventfd_signal(io_data->ffs->ffs_eventfd, 1); diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c index d3deb23eb2ab..15dff219b483 100644 --- a/drivers/usb/gadget/legacy/inode.c +++ b/drivers/usb/gadget/legacy/inode.c @@ -469,7 +469,7 @@ static void ep_user_copy_worker(struct work_struct *work) ret = -EFAULT; /* completing the iocb can drop the ctx and mm, don't touch mm after */ - iocb->ki_complete(iocb, ((u64) ret << 32) | ret); + iocb->ki_complete(iocb, aio_res_pack(ret, ret)); kfree(priv->buf); kfree(priv->to_free); @@ -499,8 +499,10 @@ static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req) kfree(priv); iocb->private = NULL; /* aio_complete() reports bytes-transferred _and_ faults */ - aio_ret = req->actual ? req->actual : (long)req->status; - aio_ret |= (u64) req->status << 32; + if (req->actual) + aio_ret = aio_res_pack(req->actual, req->status); + else + aio_ret = aio_res_pack(req->status, req->status); iocb->ki_complete(iocb, aio_ret); } else { /* ep_copy_to_user() won't report both; we hide some faults */ diff --git a/fs/aio.c b/fs/aio.c index 3674abc43788..cd43a26b2aa2 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -1442,8 +1442,8 @@ static void aio_complete_rw(struct kiocb *kiocb, u64 res) * 32-bits of value at most for either value, bundle these up and * pass them in one u64 value. */ - iocb->ki_res.res = lower_32_bits(res); - iocb->ki_res.res2 = upper_32_bits(res); + iocb->ki_res.res = (long) lower_32_bits(res); + iocb->ki_res.res2 = (long) upper_32_bits(res); iocb_put(iocb); } diff --git a/include/linux/aio.h b/include/linux/aio.h index b83e68dd006f..50a6c7da27ec 100644 --- a/include/linux/aio.h +++ b/include/linux/aio.h @@ -24,4 +24,18 @@ static inline void kiocb_set_cancel_fn(struct kiocb *req, extern unsigned long aio_nr; extern unsigned long aio_max_nr; +/* + * Take some care packing two 32-bit quantities into a 64-bit, so we don't + * get sign extensions ruining the result. aio uses long, but it's really + * just 32-bit values. + */ +static inline u64 aio_res_pack(long res, long res2) +{ + u64 ret; + + ret = (u64) res2 << 32; + ret |= (u32) res; + return ret; +} + #endif /* __LINUX__AIO_H */ -- Jens Axboe