From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4BC4841D8D; Mon, 27 Feb 2023 09:24:23 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DCF0E40A84; Mon, 27 Feb 2023 09:24:22 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 944D440A7D for ; Mon, 27 Feb 2023 09:24:21 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677486260; 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: in-reply-to:in-reply-to:references:references; bh=/1cTpWyLFeR/pUVmqKyIi1wKhMqd5SLBfX8SseGyXzc=; b=cOAT+AwB0K1/B3n6cibnIePIBw1zAHr7UVPlE8k2xwlIcD5OJaygIsGwxkp0pT1f/KB4YS Y6NYxNxrghhxwiCUdA5Quul40iMcAl52Dn/OrE7YScAXz60rsPzT5VnPNlvU9HMdXrQL4V zYki9AW46fWNUAq0eVNFiwng15xHmyU= Received: from mail-pj1-f69.google.com (mail-pj1-f69.google.com [209.85.216.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-657-_d9weYRdOcmAfu55vpreyg-1; Mon, 27 Feb 2023 03:24:19 -0500 X-MC-Unique: _d9weYRdOcmAfu55vpreyg-1 Received: by mail-pj1-f69.google.com with SMTP id i6-20020a17090a650600b002372da4819eso1289507pjj.0 for ; Mon, 27 Feb 2023 00:24:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=/1cTpWyLFeR/pUVmqKyIi1wKhMqd5SLBfX8SseGyXzc=; b=5tn1t/7TaaWdLefOQFBScC9i7DGw8sYrrV5fBtN/smkO3lgfebgM3NA7xLBlmsFp5u m9eKmBqvzw3ahTSSpitWkdgZxKV3SmZDeHXSTONUKvfhQUOYio4XQ4qJkV4Vlnd6weN/ WuvIhmaqD3LcTnHn+KcdVKafyTvDAzgEpvztzQcuZqLF26UGCzXJY6JhrfHvnPLt/nDN v7zWwHMALhmMnHgFBPgXqiF7ZHg/p6R+EiU1Nd25QrQC8xtP+TmAjc8h6PFarag0NMBR GFkW//dbCGvJLi5Cas2UI/F8DpeQxuK128PJZ13UdCfUOShNnmEZ7Ats/OXl585kth+n 7t4w== X-Gm-Message-State: AO0yUKUpfUqglzl/6ETEgmzzqyCnfqghQkVKK9fj0KJhkn7S5bb8vHir tN+DvV2GBMU/mW3jO/0GZ3q3HmID3M44jtuQU5NpRbYg2tTt5HIpgBafnoOolEflQhEGbRo02xi qqSXjeLOehKKaMx0fl/k= X-Received: by 2002:a17:903:2591:b0:19a:98ea:5ef1 with SMTP id jb17-20020a170903259100b0019a98ea5ef1mr5608944plb.13.1677486258354; Mon, 27 Feb 2023 00:24:18 -0800 (PST) X-Google-Smtp-Source: AK7set/mhl+A5p7SBstVNk8nW/tlpVRao/IkcJdZoQ4QPo/w0zo7NurHFJhMyc/DMedJiXZbSUAWCJ9FWJX3ladX5fw= X-Received: by 2002:a17:903:2591:b0:19a:98ea:5ef1 with SMTP id jb17-20020a170903259100b0019a98ea5ef1mr5608936plb.13.1677486258024; Mon, 27 Feb 2023 00:24:18 -0800 (PST) MIME-Version: 1.0 References: <20230224081642.2566619-1-david.marchand@redhat.com> <20230224151143.3274897-1-david.marchand@redhat.com> <20230224151143.3274897-12-david.marchand@redhat.com> In-Reply-To: From: David Marchand Date: Mon, 27 Feb 2023 09:24:06 +0100 Message-ID: Subject: Re: [PATCH v2 11/20] net/virtio: annotate lock for guest announce To: "Xia, Chenbo" , Maxime Coquelin Cc: "dev@dpdk.org" , "thomas@monjalon.net" X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hello Chenbo, Adding Maxime too. On Mon, Feb 27, 2023 at 3:05 AM Xia, Chenbo wrote: > > @@ -1217,7 +1217,7 @@ virtio_notify_peers(struct rte_eth_dev *dev) > > } > > > > /* If virtio port just stopped, no need to send RARP */ > > - if (virtio_dev_pause(dev) < 0) { > > + if (virtio_dev_pause(dev) != 0) { > > rte_pktmbuf_free(rarp_mbuf); > > return; > > } > > diff --git a/drivers/net/virtio/virtio_ethdev.h > > b/drivers/net/virtio/virtio_ethdev.h > > index c08f382791..ece0130603 100644 > > --- a/drivers/net/virtio/virtio_ethdev.h > > +++ b/drivers/net/virtio/virtio_ethdev.h > > @@ -112,8 +112,11 @@ int eth_virtio_dev_init(struct rte_eth_dev *eth_dev); > > > > void virtio_interrupt_handler(void *param); > > > > -int virtio_dev_pause(struct rte_eth_dev *dev); > > -void virtio_dev_resume(struct rte_eth_dev *dev); > > +#define VIRTIO_DEV_TO_HW(dev) ((struct virtio_hw *)(dev)->data- > > >dev_private) > > +int virtio_dev_pause(struct rte_eth_dev *dev) > > + __rte_exclusive_trylock_function(0, &VIRTIO_DEV_TO_HW(dev)- > > >state_lock); > > Just curious, why this is trylock instead of lock? I wrote this patch some time ago. At the time, I must say that I preferred removing those helpers (the only caller is virtio_notify_peers()). It seems those helpers were added as a kind of api for future usecases, it seemed a reason for keeping them. So I changed my mind and just annotated them. For your question, annotating with "lock" would tell clang that the function always takes the lock, regardless of the function return value. One alternative to this patch could be to always take the lock (+annotate dev_pause as "lock"), and have the caller release the lock if != 0 return value. But it seems counterintuitive to me. WDYT? -- David Marchand