From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out30-113.freemail.mail.aliyun.com (out30-113.freemail.mail.aliyun.com [115.124.30.113]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DC63228E7 for ; Tue, 21 May 2024 12:41:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=115.124.30.113 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716295274; cv=none; b=d4YI2z0dqsifDQNRPYtePcFJOAdHAk6rvUpuRu+wXRa2v1jIixtCoiBiS3ocltIEBWOSwvYkv3fmk/n7JODt7iKgGP9SADb3nta8rLDfjI45QEELUsLd6y5YQpcC5Gdd6ql+veG3ZczIdu/A59cWfuHigXf9jGkn+KOBM05Y0hw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716295274; c=relaxed/simple; bh=4FAvSIMyGafDHgVAWFqbHQ1GPYNLomU4LLCWjUo9a74=; h=Message-ID:Subject:Date:From:To:Cc:References:In-Reply-To: Content-Type; b=QRkbqIaNnCdK8fOmMa9I4N77qP8fgUwZ42hVY3ekmBrIEb2nUTGc2M+BxXNqIwLHy7O4DKKEG8qQm+hWePkJaszshhGryXz6Q9xNKICTynMwI4dhTfwojEAkMDMpJaaxJ94fVJ2FFFaByXzdvC4qi+Jz6vDDU4mDAyF4RgmquX0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.alibaba.com; spf=pass smtp.mailfrom=linux.alibaba.com; dkim=pass (1024-bit key) header.d=linux.alibaba.com header.i=@linux.alibaba.com header.b=BcUYHhuL; arc=none smtp.client-ip=115.124.30.113 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.alibaba.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.alibaba.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.alibaba.com header.i=@linux.alibaba.com header.b="BcUYHhuL" DKIM-Signature:v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.alibaba.com; s=default; t=1716295263; h=Message-ID:Subject:Date:From:To:Content-Type; bh=cb04k7tDRvZltKpQSiAAZcs4sOTX51ozfeKWytHcfd8=; b=BcUYHhuLTKIR18n7fXVvdinsiliynG1xeAUVBQvnWw/YHJzfmcBl820jaH9ixCB+ecXB1WUg0E9jWv0tiGk/4pu8rm79YChaMrTeZXvnLTgbWC/Gy9yBJrdsgW4NR7pumWzIMaZeUMzwyNqe52kwRU/g1qDzr2/9VyvXGnj9zPY= X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R211e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=maildocker-contentspam033022160150;MF=hengqi@linux.alibaba.com;NM=1;PH=DS;RN=5;SR=0;TI=SMTPD_---0W6xrSVp_1716295260; Received: from localhost(mailfrom:hengqi@linux.alibaba.com fp:SMTPD_---0W6xrSVp_1716295260) by smtp.aliyun-inc.com; Tue, 21 May 2024 20:41:02 +0800 Message-ID: <1716285826.606679-2-hengqi@linux.alibaba.com> Subject: Re: RE: RE: RE: RE: [PATCH v2] virtio-net: improve description of default coalescing parameters Date: Tue, 21 May 2024 18:03:46 +0800 From: Heng Qi To: Parav Pandit Cc: Xuan Zhuo , "virtio-comment@lists.linux.dev" , Jason Wang , "Michael S . Tsirkin" References: <20240513125634.102955-1-hengqi@linux.alibaba.com> <1715665954.5198457-1-hengqi@linux.alibaba.com> <1715673206.4680686-3-hengqi@linux.alibaba.com> <1716258261.2936878-1-hengqi@linux.alibaba.com> In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk X-Mailing-List: virtio-comment@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: On Tue, 21 May 2024 06:02:15 +0000, Parav Pandit wrote: >=20 >=20 > > From: Heng Qi > > Sent: Tuesday, May 21, 2024 7:54 AM > > On Tue, 14 May 2024 13:09:32 +0000, Parav Pandit > > wrote: > > > > > > > From: Heng Qi > > > > Sent: Tuesday, May 14, 2024 1:23 PM > > > > > > > > On Tue, 14 May 2024 07:48:55 +0000, Parav Pandit > > > > wrote: > > > > > > > > > > > > > > > > From: Heng Qi > > > > > > Sent: Tuesday, May 14, 2024 11:23 AM > > > > > > > > > > > > On Tue, 14 May 2024 03:59:52 +0000, Parav Pandit > > > > > > > > > > > > wrote: > > > > > > > Hi Heng, > > > > > > > > > > > > > > > > > > > > > > From: Heng Qi > > > > > > > > Sent: Monday, May 13, 2024 6:27 PM > > > > > > > > > > > > > > > > Currently, a device may initialize each vq's coalescing > > > > > > > > parameters to empirically non-zero values, so the > > > > > > > > description of this part is supplemented for the virtio spe= c. > > > > > > > > > > > > > > > Currently the spec says to initialize the vq's coalescing par= ameters to > > zero. > > > > > > > > > > > > > > Spec snippet: " Upon reset, a device MUST initialize all > > > > > > > coalescing > > > > > > parameters to 0." > > > > > > > > > > > > > > So I didn't understand why you say "current the device may > > initialize". > > > > > > > > > > > > > > Do you mean, you want to change the spec to say, by default > > > > > > > the device has > > > > > > chosen to set zero or non-zero default parameter as it likes. > > > > > > > (and not zero). > > > > > > > > > > > > Yes. > > > > > > > > > > > > If a device supports interrupt coalescing moderation, it usually > > > > > > has a non- zero default value to provide better performance. > > > > > When VIRTIO_NET_F_VQ_NOTF_COAL or VIRTIO_NET_F_NOTF_COAL is > > not > > > > negotiated, the device can apply any notification coalescing value. > > > > > (assuming EVENT_IDX is off). > > > > > > > > > > But when they above features are negotiated, it is expected that > > > > > driver is going to driver it, > > > > > > > > The driver drives the runtime parameters without conflicting with > > > > the initial default values. > > > > And most users/drivers without dim enable may not drive it. > > > > > > > There is no conflict in runtime or default parameters. > > > For driver it starts after 10msec or 4 minutes, both are runtime para= meters. > > > > > > > The device need to be defensive about default performance. > > > > > > > The device can surely offer the default performance with non zero def= ault > > values. > >=20 > > That's what I'm mentioning. > >=20 > > > > > > > >in such case what is the good motivation to start with some arbitr= ary > > value? > > > > > > > > > > > I'm trying to fix this, and > > > > > > we shouldn't force the device to always initialize with 0. > > > > > > > > > > > If driver is in control, driver decides what values to set. > > > > > And hence, arbitrary value is > > > > > > > > Then maybe the driver is never set and the device uses a 0 which > > > > messes up most data scenarios. > > > > > > > If driver does not want to set it, why did it enable the feature in f= irst place? > > > So asking again, is it because driver enabled featured too early in t= he driver > > load sequence, and not DIM is also disabled? > >=20 > > I don't see the connection. > > > There is a connection. >=20 > Basically, what you want is, >=20 > 1. Driver negotiates feature bits during driver load. >=20 > 2. Driver or user sometimes never configures the right coalescing paramet= ers. >=20 > 3. Device to continue to apply the best coalescing parameters, typically = keeps changing based on the workload. Wait. Why does the device keeping changing the coalescing parameters if the driver does not modify parameters? Do you mean some kind of hw-DIM running on the device? > The device to keep doing this until the driver takes the control of doing= it by-itself.=20 > For example via net-dim enablement or user doing via ethtool. Right. >=20 > So what is needed is, not just have _one_ default value, but the device a= bility to constantly applying notification coalescing as desired. The "default parameters" I'm talking about is the "initial parameters", are= we on the same line? >=20 > When the driver issues the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command for t= he first time onwards, at that point, the device receives the hint/indicati= on, > That now on, driver is in the control, hence the device should stop apply= ing its own logic. >=20 > Just one default is not enough. > And feature negotiation is of no help here at such an early stage. >=20 > This is the limitation of feature bits negotiation and for now we need to= live it it. >=20 > And heavy alternative that I prefer to avoid is keep the notification fea= tures disabled at default, re-initialize the driver when user enables the n= otification coaleasing. > However, this is disruptive enough. >=20 > So I propose that we draft the spec in device requirements as:. >=20 > The device MAY apply any notification coalescing values when VIRTIO_NET_F= _VQ_NOTF_COAL or VIRTIO_NET_F_NOTF_COAL is negotiated, until driver instruc= t the values for the first time. > Once the driver has performed VIRTIO_NET_F_VQ_NOTF_COAL command for a VQ = or VIRTIO_NET_F_NOTF_COAL command for set of VQs, after that the device SHO= ULD generate notifications based on driver supplied parameters. I've always agreed with this. >=20 > > > > > > If feature is negotiated, than DIM can be enabled by default, and the= refore > > there is no need for the defaults? > >=20 > > Right. But if DIM is not enabled (pls remember that DIM is just one of = the > > application scenarios of VQ_NOTF_COAL, right?), the user can query the > > initial value of the device, which may be any value. > > > It is going to be just more than initial value, as device will have to ke= ep changing the value. > =20 > > > > > > Or you are saying, that enabling DIM as default has some issue, and u= ntil > > that point you prefer to have some default in the device? > >=20 > > Partly true. The complete purpose is that the device can have any defau= lt > > values > > (0 or non-zero). > >=20 > > > > > > > Cloud vendors cannot do this. And I don=E2=80=99t seem to see any > > > > interference with other devices from this proposal? > > > > If the device does not want a non-zero value, a value of 0 can be > > > > used as the default value. > > > > > > > I didn=E2=80=99t understand above. > > > > > > > > > > > > > > Especially when the DIM is disabled from the enabled state, > > > > > > allowing the driver to restore default parameters for device > > > > > > will prevent occasional performance degradation. > > > > > > > > > > > When DIM is disabled, do you mean driver does not have any good > > defaults? > > > > > > > > DIM is not guaranteed to have a friendly value when disabled. > > > > > > > I didn=E2=80=99t follow. > > > > > > (a) When DIM is enabled, DIM decides the value. > > > (b) When DIM is disabled, zero is not good default in the device. Did= I > > understand right? > > > > > > I agree with (a) and (b). > >=20 > > Right. > >=20 > > > > > > > > I guess this issue surfaces from the feature negotiation > > > > > limitation that, it > > > > must be done early enough before DRIVER_OK. > > > > > And DIM is still disabled in driver. > > > > > > > > We lack the capability filed of default coalescing parameters, so > > > > GET is used to alleviate this. > > > > > > > When GET is not supported, ethtool should get -ENOSUPP instead of 0. > >=20 > > We don't have a separate feature bit for GET, right? So if VQ_NOTF_COAL= is > > negotiated, the device supports the GET command. > > > Yes, I meant to say that VIRTIO_NET_CTRL_NOTF_COAL and VQ_NOTF_COAL both = may not be offered. > When only VIRTIO_NET_CTRL_NOTF_COAL is done, the driver can report -ENOSU= PP. >=20 I think "VIRTIO_NET_F_NOTF_COAL" and VQ_NOTF_COAL both may not be offered? > =20 > > > > > > > > > > > > > That explains. > > > > > > > > > > Commit log needs to explain this limitation and the gain by > > > > > relaxing it to be > > > > non-zero. > > > > > > > > > > Any reason DIM is disabled by default in the driver? > > > > > If VQ_NOTF_COAL is supported by device, DIM should be enabled by > > > > default. > > > > > > > > virtio-net support for netdim needs to be optimized. Of course, I > > > > also found that other network cards also have optimization points. > > > > > > > > I'm trying to enable DIM for upstream by default, after I've done > > > > the profile list tuning etc. > > > > > > > > > > > > > > > > > > > > > > > > Since the VIRTIO_NET_F_NOTF_COAL feature does not provide a > > > > > > > > related GET command or config field, we still consider its > > > > > > > > default values > > > > to be 0. > > > > > > > > > > > > > > > The behavior for VIRTIO_NET_F_NOTF_COAL and > > > > > > VIRTIO_NET_F_VQ_NOTF_COAL can stay same for the default value. > > > > > > > Can you please explain the motivation for running different > > > > > > > default values > > > > > > for two different features in this commit log? > > > > > > > > > > > > The current spec leaves coalescing parameters for both features= at 0. > > > > > > > > > > > > F_VQ_NOTF_COAL provides a GET command, that is, non-zero 0 can > > > > > > be obtained by the driver from the device, but F_NOTF_COAL does > > > > > > not have a similar GET command. > > > > > > > > > > > GET command doesn't give the ability to define the defaults. > > > > > So both can have non zero defaults. > > > > > > > > Makes sense. > > > > > > > > > > > > > > > So when vq reset occurs or dim is disabled from the enabled > > > > > > state, the driver with F_VQ_NOTF_COAL negotiated has the ability > > > > > > to restore non-zero values, but only when F_NOTF_COAL is > > > > > > negotiated, the driver has no path to obtain the non-zero value, > > > > > > let alone restore non-zero > > > > values. > > > > > > > > > > > When F_NOTF_COAL is negotiated, if the device has its non zero > > > > > defaults, > > > > why does driver need to restore anything? > > > > > > > > Yes, device will set these. > > > > > > > Ok. so no restore needed. > > > > > > > > > > > > > > > > > > Suggested-by: Jason Wang > > > > > > > > Signed-off-by: Heng Qi > > > > > > > > --- > > > > > > > > v1->v2: > > > > > > > > - Update description @Jason. > > > > > > > > > > > > > > > > device-types/net/description.tex | 22 > > > > > > > > +++++++++++++++------- > > > > > > > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > diff --git a/device-types/net/description.tex b/device- > > > > > > > > types/net/description.tex index 61cce1f..cf4b12c 100644 > > > > > > > > --- a/device-types/net/description.tex > > > > > > > > +++ b/device-types/net/description.tex > > > > > > > > @@ -1805,6 +1805,10 @@ \subsubsection{Control > > > > > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi > > > > > > > > > > > > > > > > The device may generate notifications more or less > > > > > > > > frequently than specified by set commands of the > > > > > > > > VIRTIO_NET_CTRL_NOTF_COAL > > > > class. > > > > > > > > > > > > > > > > +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is offered, the > > > > > > > > +device may initialize the coalescing parameters for each > > > > > > > > +transmit or receive virtqueue to non-zero values, > > > > > > > > +otherwise, to 0, which are called default > > > > > > > > coalescing parameters. > > > > > > > > + > > > > > > > Instead of 'offered', it should be 'negotiated'. Because if > > > > > > > may be offered, > > > > > > but driver may not have enabled it. > > > > > > > > > > > > I do not think so. > > > > > > > > > > > > Even if the driver does not negotiate, as long as the device has > > > > > > the ability to adjust coalescing parameters (i.e. offered), the > > > > > > device will initialize any values. > > > > > > > > > > > The device can initialize coalescing parameters regardless of > > > > > VQ_NOTF_COAL > > > > offered or not. > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > > > > > > > > > If coalescing parameters are being set, the device applies > > > > > > > > the last coalescing parameters set for a virtqueue, > > > > > > > > regardless of the command used to set the parameters. Use > > > > > > > > the following command sequence with two pairs of virtqueue= s as > > an example: > > > > > > > > @@ -1873,6 +1877,10 @@ \subsubsection{Control > > > > > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi > > > > > > > > > > > > > > > > The driver MUST ignore the values of coalescing parameters > > > > > > > > received from the VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET > > command > > > > > > > > if the device responds with VIRTIO_NET_ERR. > > > > > > > > > > > > > > > > +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated, the > > > > > > > > +driver > > > > > > > > MUST > > > > > > > > +get coalescing parameters for each enabled transmit or > > > > > > > > +receive virtqueue through the > > > > > > > > +VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET > > > > > > command > > > > > > > > after a successful device reset. > > > > > > > > + > > > > > > > The driver is free to not call GET call at all. It can always= just do set. > > > > > > > So it cannot be must requirement for the driver to get them. > > > > > > > > > > > > This is mainly forced to MUST during the probe phase. > > > > > > Other timings, the existing spec has already described it clear= ly. > > > > > > > > > > > I don't see it is needed to force driver to read. > > > > > Why is it must for the driver to read it? It can operate without = read. > > > > > > > > Otherwise the device has a non-zero default value, but users using > > > > ethtool -c see a value of 0. > > > > > > > When GET is supported, ethtool callback should query the current valu= e. > > > Driver can read it once at start time, but it is not a MUST requireme= nt. > > > Driver can read it runtime too. It is the driver implementation choic= e. > > > We don=E2=80=99t need to tell in the spec, when driver MUST read it. > >=20 > > I agree with this. > >=20 > > > > > > > > > > > > > > > > > > > > > > The wording should be, the driver MAY get ... > > > > > > > > > > > > > > > \devicenormative{\subparagraph}{Notifications > > > > > > > > Coalescing}{Device Types / Network Device / Device Operation > > > > > > > > / Control Virtqueue / Notifications Coalescing} > > > > > > > > > > > > > > > > The device MUST ignore \field{reserved}. > > > > > > > > @@ -1884,18 +1892,18 @@ \subsubsection{Control > > > > > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi > > > > > > > > The device MUST respond to > > VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET > > > > > > > > and VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET commands with > > > > > > VIRTIO_NET_ERR if > > > > > > > > the designated virtqueue is not an enabled transmit or > > > > > > > > receive > > > > virtqueue. > > > > > > > > > > > > > > > > -Upon disabling and re-enabling a transmit virtqueue, the > > > > > > > > device MUST set the coalescing parameters of the virtqueue > > > > > > > > -to those configured through the > > > > > > > > VIRTIO_NET_CTRL_NOTF_COAL_TX_SET > > > > > > command, or, > > > > > > > > if the driver did not set any TX coalescing parameters, to = 0. > > > > > > > > - > > > > > > > > -Upon disabling and re-enabling a receive virtqueue, the > > > > > > > > device MUST set the coalescing parameters of the virtqueue > > > > > > > > -to those configured through the > > > > > > > > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET > > > > command, > > > > > > > > or, if the driver did not set any RX coalescing parameters,= to 0. > > > > > > > > - > > > > > > > > The behavior of the device in response to set commands of > > > > > > > > the VIRTIO_NET_CTRL_NOTF_COAL class is best-effort: > > > > > > > > the device MAY generate notifications more or less > > > > > > > > frequently than specified. > > > > > > > > > > > > > > > > A device SHOULD NOT send used buffer notifications to the > > > > > > > > driver if the notifications are suppressed, even if the > > > > > > > > notification conditions are > > > > > > met. > > > > > > > > > > > > > > > > -Upon reset, a device MUST initialize all coalescing parame= ters to > > 0. > > > > > > > > +Upon reset, a device MUST set default coalescing parameters > > > > > > > > +for all transmit or receive virtqueues. > > > > > > > > + > > > > > > > The device MAY set parameters to zero or non zero values.. > > > > > > > > > > > > Ok. > > > > > > > > > > > > > I am still missing the motivation part of why to set non zero > > > > > > > value, even if > > > > > > the driver has negotiated the feature. > > > > > > > > > > > > Please see the above description. > > > > > > > > > > > If I understood is right, the case is: > > > > > _VQ_NOTF_COAL is negotiated, and DIM is disabled. > > > > > And device is unable to apply some good non zero defaults. > > > > > > > > > > > > If _VQ_NOTF_COAL is negotiated, and DIM is disabled: > > > > > > > > 1. The device wants to set a non-zero value for good perf, > > > > but the user sees a value of 0. > > > > > > > The only limitation that we want to remove is, on device reset, the d= evice > > may have any default value. > >=20 > > Right, and the driver may query the value using the GET command. > >=20 > > > > > > > 2. When the user has not actively modified the value, DIM switches = from > > the > > > > enabled state to the disabled state, then the driver expects to = issue a > > > > setting command with a default value. To avoid performance > > degradation > > > > caused by a bad value. > > > > > > > This #2 seems like some hack. Should I read the Linux code in this ar= ea? > >=20 > > No. This is some kind of performance optimization at the code level. Wh= en > > DIM is turned off, the driver configures default values for the device = to avoid > > any big performance regression. > > > If the driver configures the default, we don=E2=80=99t need device to app= ly any defaults. when does the driver configure the default value (actually I mean the initi= al value) and how does the driver know which default (initial value) value app= lies to all devices? Thanks. > I thought your intention was, the driver does not know what is the best n= otification rate to configure and feature is enabled in the device. > And the device does not have any good defaults (because driver is not con= figuring any defaults). >=20 > But here you say, the driver configures the drefault. I am lost. =E2=98= =B9 > =20 > > > > > > > We assume that most users do not have the prerequisite knowledge to > > > > modify coalecing parameters. > > > > > > > Right. Hence the DIM should be used. > > > When DIM is not used, a non zero default is good to have. > > > (without forcing the driver to read it). > >=20 > > +1 > >=20 > > Thanks. > >=20 > > > > > > > Thanks. > > > > > > > > > > > > > > Right? > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > +Regardless of whether the coalescing parameters have been > > > > > > > > +overridden by set commands from the > > > > VIRTIO_NET_CTRL_NOTF_COAL > > > > > > > > +class, upon > > > > > > > > disabling > > > > > > > > +and re-enabling a transmit or receive virtqueue, the device > > > > > > > > +MUST set the previous parameters for the virtqueue. > > > > > > > > > > > > > > > > \paragraph{Device Statistics}\label{sec:Device Types / > > > > > > > > Network Device / Device Operation / Control Virtqueue / > > > > > > > > Device Statistics} > > > > > > > > > > > > > > > > -- > > > > > > > > 2.32.0.3.g01195cf9f > > > > > > > > > > >=20