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 X-Spam-Level: X-Spam-Status: No, score=-5.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 80465C433E0 for ; Mon, 18 Jan 2021 15:19:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3475022C7E for ; Mon, 18 Jan 2021 15:19:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2405399AbhARPSi (ORCPT ); Mon, 18 Jan 2021 10:18:38 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:60467 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2405161AbhARPS3 (ORCPT ); Mon, 18 Jan 2021 10:18:29 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610983022; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=psrfvoG3s6Q5vxt3L2I9TwaBLHc6rZJ8JZ9ILZMQQLQ=; b=JuvWhx5oP7Yc3rdL+gESQ/2QsljbZPwx/i7widicX6kLayT2feEe/9tOo1bnUdhnYcfYHF lO20Fd17husvOABD7+sQ3GETKJlBbMXfIupM64Lfn2PgOVzzXOaBWUlhfycv/hnMGRbfc0 /uxhX7Bq4pu11JUfpJJd1ZG/H9NzoNg= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-69-v-sUbFVyP9iMdrjO7pYWNA-1; Mon, 18 Jan 2021 10:17:00 -0500 X-MC-Unique: v-sUbFVyP9iMdrjO7pYWNA-1 Received: by mail-wr1-f72.google.com with SMTP id b8so8429751wrv.14 for ; Mon, 18 Jan 2021 07:17:00 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=psrfvoG3s6Q5vxt3L2I9TwaBLHc6rZJ8JZ9ILZMQQLQ=; b=gkqrYPt1oI7KG5r98Xa/XuvSDkswkZ7ULGYO1iiFRfiMbK/tA0wTw7FOFlmRZ8Fm0a JFVQzMvwqO51CiYdH8zJwGkOsbG/4Hx3U2+4aOpyIXjzAXoWpgAddrJsymla557kzsG6 Jhekc8XRBBcGBMTrC1T/bUmeUSiJMookZ0e3vDD/z3N+1FnTZWoKi3ZuYPdldB6IKsSE z7MuS17IPfbEn0CCJivx4G5J+DWTETOvBHkOmFQJBIIdopgCl/reYoNRyagWdFFv/5f0 6nKi6cRQxaaEopUffetFuEoYPa+ZsSPAcOJ3vWnFCS2xlm7F/rpCLDQY4nsKT798blQk JVOQ== X-Gm-Message-State: AOAM53081nuvKZmzrFxE+HzwgmTy8Zczr1xO/j4Ng6qkSriX2w+rDzJo pIuxTfuwoXljkX6MasGIeeNmmHFKaAa0j9piBnHXcx98+buqf7Xg3O5ePIg7sKQrMFhKSEbG4Dl hNYTcGLWRlW7O X-Received: by 2002:adf:e54a:: with SMTP id z10mr26882327wrm.1.1610983019797; Mon, 18 Jan 2021 07:16:59 -0800 (PST) X-Google-Smtp-Source: ABdhPJxXG3dAk6icAdSs8xnG72+nZUruxeveo9BolCI0f5eiHmFFp/RlmpfjavbAYMBXV+ShJ7Tvbw== X-Received: by 2002:adf:e54a:: with SMTP id z10mr26882310wrm.1.1610983019631; Mon, 18 Jan 2021 07:16:59 -0800 (PST) Received: from steredhat (host-79-34-249-199.business.telecomitalia.it. [79.34.249.199]) by smtp.gmail.com with ESMTPSA id z130sm28028318wmb.33.2021.01.18.07.16.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Jan 2021 07:16:58 -0800 (PST) Date: Mon, 18 Jan 2021 16:16:56 +0100 From: Stefano Garzarella To: Arseny Krasnov , stsp Cc: Stefan Hajnoczi , "Michael S. Tsirkin" , Jason Wang , "David S. Miller" , Jakub Kicinski , Colin Ian King , Andra Paraschiv , Jeff Vander Stoep , kvm , Linux Virtualization , netdev , kernel list , Krasnov Arseniy Subject: Re: [RFC PATCH v2 00/13] virtio/vsock: introduce SOCK_SEQPACKET support. Message-ID: References: <20210115053553.1454517-1-arseny.krasnov@kaspersky.com> <2fd6fc75-c534-7f70-c116-50b1c804b594@yandex.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2fd6fc75-c534-7f70-c116-50b1c804b594@yandex.ru> Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Fri, Jan 15, 2021 at 12:59:30PM +0300, stsp wrote: >15.01.2021 08:35, Arseny Krasnov пишет: >> This patchset impelements support of SOCK_SEQPACKET for virtio >>transport. >> As SOCK_SEQPACKET guarantees to save record boundaries, so to >>do it, new packet operation was added: it marks start of record (with >>record length in header), such packet doesn't carry any data. To send >>record, packet with start marker is sent first, then all data is sent >>as usual 'RW' packets. On receiver's side, length of record is known >>from packet with start record marker. Now as packets of one socket >>are not reordered neither on vsock nor on vhost transport layers, such >>marker allows to restore original record on receiver's side. If user's >>buffer is smaller that > >than > > >> record length, when > >then > > >> v1 -> v2: >> - patches reordered: af_vsock.c changes now before virtio vsock >> - patches reorganized: more small patches, where +/- are not mixed > >If you did this because I asked, then this >is not what I asked. :) >You can't just add some static func in a >separate patch, as it will just produce the >compilation warning of an unused function. >I only asked to separate the refactoring from >the new code. I.e. if you move some code >block to a separate function, you shouldn't >split that into 2 patches, one that adds a >code block and another one that removes it. >It should be in one patch, so that it is clear >what was moved, and no new warnings are >introduced. >What I asked to separate, is the old code >moves with the new code additions. Such >things can definitely go in a separate patches. Arseny, thanks for the v2. I appreciated that you moved the af_vsock changes before the transport and also the test, but I agree with stsp about split patches. As stsp suggested, you can have some "preparation" patches that touch the already existing code (e.g. rename vsock_stream_sendmsg in vsock_connectible_sendmsg() and call it inside the new vsock_stream_sendmsg, etc.), then a patch that adds seqpacket stuff in af_vsock. Also for virtio/vhost transports, you can have some patches that add support in virtio_transport_common, then a patch that enable it in virtio_transport and a patch for vhost_vsock, as you rightly did in patch 12. So, I'd suggest moving out the code that touches virtio_transport.c from patch 11. These changes should simplify the review. In addition, you can also remove the . from the commit titles. I left other comments in the single patches. Thanks, Stefano