* [PATCH 05/10] drm/appletbdrm: Allocate request/response buffers in begin_fb_access
From: Thomas Zimmermann @ 2026-05-07 7:12 UTC (permalink / raw)
To: mripard, maarten.lankhorst, airlied, airlied, simona, admin,
gargaditya08, paul, zack.rusin, bcm-kernel-feedback-list
Cc: dri-devel, linux-hyperv, intel-gfx, intel-xe, linux-mips,
virtualization, Thomas Zimmermann
In-Reply-To: <20260507075725.29738-1-tzimmermann@suse.de>
In atomic_check, damage handling is not fully evaluated. Another
atomic_check helper could trigger a full modeset and thus invalidate
damage clips.
Allocation of the request/response buffers in appletbdrm depends on
correct damage information. Otherwise it might allocate incorrectly
sized buffers. Allocate the buffers in the driver's begin_fb_access
helper. It runs early during the commit when damage clipping has been
fully evaluated.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/tiny/appletbdrm.c | 50 +++++++++++++++++++------------
1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
index 278bb23fe4c8..a7eba6880ba5 100644
--- a/drivers/gpu/drm/tiny/appletbdrm.c
+++ b/drivers/gpu/drm/tiny/appletbdrm.c
@@ -315,33 +315,21 @@ static const u32 appletbdrm_primary_plane_formats[] = {
DRM_FORMAT_XRGB8888, /* emulated */
};
-static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
- struct drm_atomic_state *state)
+static int appletbdrm_primary_plane_helper_begin_fb_access(struct drm_plane *plane,
+ struct drm_plane_state *new_plane_state)
{
- struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
- struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
- struct drm_crtc *new_crtc = new_plane_state->crtc;
- struct drm_crtc_state *new_crtc_state = NULL;
struct appletbdrm_plane_state *appletbdrm_state = to_appletbdrm_plane_state(new_plane_state);
+ size_t frames_size = 0;
struct drm_atomic_helper_damage_iter iter;
struct drm_rect damage;
- size_t frames_size = 0;
size_t request_size;
int ret;
- if (new_crtc)
- new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
-
- ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
- DRM_PLANE_NO_SCALING,
- DRM_PLANE_NO_SCALING,
- false, false);
+ ret = drm_gem_begin_shadow_fb_access(plane, new_plane_state);
if (ret)
return ret;
- else if (!new_plane_state->visible)
- return 0;
- drm_atomic_helper_damage_iter_init(&iter, old_plane_state, new_plane_state);
+ drm_atomic_helper_damage_iter_init(&iter, NULL, new_plane_state);
drm_atomic_for_each_plane_damage(&iter, &damage) {
frames_size += struct_size((struct appletbdrm_frame *)0, buf, rect_size(&damage));
}
@@ -369,6 +357,29 @@ static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
return 0;
}
+static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_state *state)
+{
+ struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+ struct drm_crtc *new_crtc = new_plane_state->crtc;
+ struct drm_crtc_state *new_crtc_state = NULL;
+ int ret;
+
+ if (new_crtc)
+ new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
+
+ ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, false);
+ if (ret)
+ return ret;
+ else if (!new_plane_state->visible)
+ return 0;
+
+ return 0;
+}
+
static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
struct drm_plane_state *old_state,
struct drm_plane_state *state)
@@ -468,7 +479,7 @@ static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
}
static void appletbdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
- struct drm_atomic_state *old_state)
+ struct drm_atomic_state *old_state)
{
struct appletbdrm_device *adev = drm_to_adev(plane->dev);
struct drm_device *drm = plane->dev;
@@ -552,7 +563,8 @@ static void appletbdrm_primary_plane_destroy_state(struct drm_plane *plane,
}
static const struct drm_plane_helper_funcs appletbdrm_primary_plane_helper_funcs = {
- DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+ .begin_fb_access = appletbdrm_primary_plane_helper_begin_fb_access,
+ .end_fb_access = drm_gem_end_shadow_fb_access,
.atomic_check = appletbdrm_primary_plane_helper_atomic_check,
.atomic_update = appletbdrm_primary_plane_helper_atomic_update,
.atomic_disable = appletbdrm_primary_plane_helper_atomic_disable,
--
2.54.0
^ permalink raw reply related
* [PATCH 02/10] drm/atomic-helpers: Evaluate plane damage after atomic_check
From: Thomas Zimmermann @ 2026-05-07 7:12 UTC (permalink / raw)
To: mripard, maarten.lankhorst, airlied, airlied, simona, admin,
gargaditya08, paul, zack.rusin, bcm-kernel-feedback-list
Cc: dri-devel, linux-hyperv, intel-gfx, intel-xe, linux-mips,
virtualization, Thomas Zimmermann
In-Reply-To: <20260507075725.29738-1-tzimmermann@suse.de>
Each plane's and CRTC's atomic_check might trigger a full modeset. As
this affects the plane's damage handling, evaluate damage clips after
running the atomic_check helpers.
Examples can be found in a number of drivers, such as ast, gud, ingenic,
mgag200 or vmwgfx, which all set mode_changed in the CRTC state to true.
Ingenic even reevaluates damage information in its plane's atomic_check.
Doing this after the atomic_check helpers ran benefits all drivers.
There's already a damage evaluation before the calls to atomic_check.
With a few fixes to drivers, this can be removed.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a768398a1884..58edd122b922 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1065,6 +1065,10 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
}
}
+ for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+ drm_atomic_helper_check_plane_damage(state, new_plane_state);
+ }
+
return ret;
}
EXPORT_SYMBOL(drm_atomic_helper_check_planes);
--
2.54.0
^ permalink raw reply related
* [PATCH 03/10] drm/ingenic: Remove calls to drm_atomic_helper_check_plane_damage()
From: Thomas Zimmermann @ 2026-05-07 7:12 UTC (permalink / raw)
To: mripard, maarten.lankhorst, airlied, airlied, simona, admin,
gargaditya08, paul, zack.rusin, bcm-kernel-feedback-list
Cc: dri-devel, linux-hyperv, intel-gfx, intel-xe, linux-mips,
virtualization, Thomas Zimmermann
In-Reply-To: <20260507075725.29738-1-tzimmermann@suse.de>
Atomic helpers call drm_atomic_helper_check_plane_damage() after the
atomic_check anyway. See atomic_helper_check_planes(). Remove the calls
from the planes' atomic_check.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 3 ---
drivers/gpu/drm/ingenic/ingenic-ipu.c | 8 ++------
2 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 70088db34f69..0a42aff79d0e 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -519,9 +519,6 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
old_plane_state->fb->format->format != new_plane_state->fb->format->format))
crtc_state->mode_changed = true;
- if (priv->soc_info->map_noncoherent)
- drm_atomic_helper_check_plane_damage(state, new_plane_state);
-
return 0;
}
diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index 34545b9c8c33..81b0fc0fa11f 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -594,7 +594,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
if (!new_plane_state->crtc ||
!crtc_state->mode.hdisplay || !crtc_state->mode.vdisplay)
- goto out_check_damage;
+ return 0;
/* Plane must be fully visible */
if (new_plane_state->crtc_x < 0 || new_plane_state->crtc_y < 0 ||
@@ -611,7 +611,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
return -EINVAL;
if (!osd_changed(new_plane_state, old_plane_state))
- goto out_check_damage;
+ return 0;
crtc_state->mode_changed = true;
@@ -645,10 +645,6 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
ipu_state->denom_w = denom_w;
ipu_state->denom_h = denom_h;
-out_check_damage:
- if (ingenic_drm_map_noncoherent(ipu->master))
- drm_atomic_helper_check_plane_damage(state, new_plane_state);
-
return 0;
}
--
2.54.0
^ permalink raw reply related
* [PATCH 01/10] drm/damage-helper: Do not alter damage clips on modeset, but ignore them
From: Thomas Zimmermann @ 2026-05-07 7:12 UTC (permalink / raw)
To: mripard, maarten.lankhorst, airlied, airlied, simona, admin,
gargaditya08, paul, zack.rusin, bcm-kernel-feedback-list
Cc: dri-devel, linux-hyperv, intel-gfx, intel-xe, linux-mips,
virtualization, Thomas Zimmermann, stable
In-Reply-To: <20260507075725.29738-1-tzimmermann@suse.de>
User space supplies rectangles for damage clipping in a plane property.
For full mode sets, we still require a full plane update. In this case,
leave the information as-is and set the ignore_damage_clips flag instead.
The damage iterator will later ignore any damage information.
Also fixes a bug where ignore_damage_clips was not cleared across plane
duplications.
Leaving the damage information as-is might be helpful to drivers that
benefit from it even on full modesets (e.g., for cache management). It
will also help with consolidating the damage-handling logic.
Also add a new unit test that evaluates the ignore_damage_clips flag. It
sets two damage clips plus the flag and tests if the reported damage
covers the entire framebuffer.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 35ed38d58257 ("drm: Allow drivers to indicate the damage helpers to ignore damage clips")
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v6.10+
---
drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
drivers/gpu/drm/drm_damage_helper.c | 6 ++--
.../gpu/drm/tests/drm_damage_helper_test.c | 28 +++++++++++++++++++
3 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 76746ad4a1bb..42f25b273dac 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -359,6 +359,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
state->fence = NULL;
state->commit = NULL;
state->fb_damage_clips = NULL;
+ state->ignore_damage_clips = false;
state->color_mgmt_changed = false;
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index 1b6850aa1688..85e20f83f757 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -78,10 +78,8 @@ void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
if (WARN_ON(!crtc_state))
return;
- if (drm_atomic_crtc_needs_modeset(crtc_state)) {
- drm_property_blob_put(plane_state->fb_damage_clips);
- plane_state->fb_damage_clips = NULL;
- }
+ if (drm_atomic_crtc_needs_modeset(crtc_state))
+ plane_state->ignore_damage_clips = true;
}
}
EXPORT_SYMBOL(drm_atomic_helper_check_plane_damage);
diff --git a/drivers/gpu/drm/tests/drm_damage_helper_test.c b/drivers/gpu/drm/tests/drm_damage_helper_test.c
index 0df2e1a54b0d..64f038a62ffe 100644
--- a/drivers/gpu/drm/tests/drm_damage_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_damage_helper_test.c
@@ -603,6 +603,33 @@ static void drm_test_damage_iter_damage_not_visible(struct kunit *test)
KUNIT_EXPECT_EQ_MSG(test, num_hits, 0, "Should not return any damage.");
}
+static void drm_test_damage_iter_damage_ignore(struct kunit *test)
+{
+ struct drm_damage_mock *mock = test->priv;
+ struct drm_atomic_helper_damage_iter iter;
+ struct drm_property_blob damage_blob;
+ struct drm_mode_rect damage[2];
+ struct drm_rect clip;
+ u32 num_hits = 0;
+
+ set_plane_src(&mock->old_state, 0, 0, 1024 << 16, 768 << 16);
+ set_plane_src(&mock->state, 0, 0, 1024 << 16, 768 << 16);
+ /* 2 damage clips, but ignore them. */
+ set_damage_clip(&damage[0], 20, 30, 200, 180);
+ set_damage_clip(&damage[1], 240, 200, 280, 250);
+ set_damage_blob(&damage_blob, &damage[0], sizeof(damage));
+ set_plane_damage(&mock->state, &damage_blob);
+ mock->state.ignore_damage_clips = true;
+ drm_atomic_helper_damage_iter_init(&iter, &mock->old_state, &mock->state);
+ drm_atomic_for_each_plane_damage(&iter, &clip) {
+ if (num_hits == 0)
+ check_damage_clip(test, &clip, 0, 0, 1024, 768);
+ num_hits++;
+ }
+
+ KUNIT_EXPECT_EQ_MSG(test, num_hits, 1, "Should return full-framebuffer damage.");
+}
+
static struct kunit_case drm_damage_helper_tests[] = {
KUNIT_CASE(drm_test_damage_iter_no_damage),
KUNIT_CASE(drm_test_damage_iter_no_damage_fractional_src),
@@ -625,6 +652,7 @@ static struct kunit_case drm_damage_helper_tests[] = {
KUNIT_CASE(drm_test_damage_iter_damage_one_outside),
KUNIT_CASE(drm_test_damage_iter_damage_src_moved),
KUNIT_CASE(drm_test_damage_iter_damage_not_visible),
+ KUNIT_CASE(drm_test_damage_iter_damage_ignore),
{ }
};
--
2.54.0
^ permalink raw reply related
* [PATCH 00/10] drm: Improve logic behind damage handling
From: Thomas Zimmermann @ 2026-05-07 7:12 UTC (permalink / raw)
To: mripard, maarten.lankhorst, airlied, airlied, simona, admin,
gargaditya08, paul, zack.rusin, bcm-kernel-feedback-list
Cc: dri-devel, linux-hyperv, intel-gfx, intel-xe, linux-mips,
virtualization, Thomas Zimmermann
DRM clients can supply information on framebuffer areas to update as
part of each page flip, called damage clipping rectangles. But DRM's
processing of this information is inconsistent and prone to errors.
- There are multiple fields and tests that decide if damage clips
should be taken or ignored.
- Sometimes, damage clips are removed behind the back of the DRM client.
- Atomic helpers evaluate damage clipping in the middle of the atomic
check: after connectors and encoders, but before planes and CRTCs. Hence
pipeline stages have an inconsistent view.
- Which leads to drivers (ingenic) doing a re-evaluation if necessary.
- Tests of plane source coordinates only happen during commits. At this
point, the driver should already know if damage clips are to be taken or
not. Because of this, some drivers (appletbdrm) might operate on incorrect
damage information for their internal workings. This also leads to excessive
use of the old plane state.
Got through DRM helpers and drivers to fix up the logic.
- Run all of the atomic checks with the damage information supplied by
DRM clients. Afterwards evaluate plane and CRTC states on whether to
take or ignore damage clips. Do all related tests in a single atomic
helper.
- Do not discard damage clips. Set ignore_damage_clips in struct
drm_plane_state instead. This includes changes to plane source-coordinates.
The damage iterator now only has to look at this flag to detect if it
should use the damage clips.
- Go over drivers and fix up the damage handling in the plane's
atomic_update helpers. Most drivers no longer need the old plane state
in their update.
- The appletbdrm driver require a fix in how it uses damage information.
Ingenic and vmwgfx can be simplified. These changes make the drivers better
structured.
- Kunit tests require some changes. Drop some obsolete tests and add a new
one for ignore_damage_flags.
Tested with bochs, mgag200, Kunit tests.
Thomas Zimmermann (10):
drm/damage-helper: Do not alter damage clips on modeset, but ignore
them
drm/atomic-helpers: Evaluate plane damage after atomic_check
drm/ingenic: Remove calls to drm_atomic_helper_check_plane_damage()
drm/damage-helper: Test src coord in
drm_atomic_helper_check_plane_damage()
drm/appletbdrm: Allocate request/response buffers in begin_fb_access
drm/damage-helper: Remove old state from
drm_atomic_helper_damage_iter_init()
drm/damage-helper: Remove old state from
drm_atomic_helper_damage_merged()
drm/atomic_helper: Do not evaluate plane damage before atomic_check
drm/damage-helper: Rename state parameters in damage helpers
drm/vmwgfx: Remove unused field struct
vmwgfx_du_update_plane.old_state
drivers/gpu/drm/ast/ast_cursor.c | 3 +-
drivers/gpu/drm/ast/ast_mode.c | 2 +-
drivers/gpu/drm/drm_atomic_helper.c | 6 +-
drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
drivers/gpu/drm/drm_damage_helper.c | 44 ++--
drivers/gpu/drm/drm_fb_dma_helper.c | 2 +-
drivers/gpu/drm/drm_mipi_dbi.c | 3 +-
drivers/gpu/drm/gud/gud_pipe.c | 3 +-
drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 3 +-
drivers/gpu/drm/i915/display/intel_plane.c | 11 +-
drivers/gpu/drm/i915/display/intel_psr.c | 3 +-
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 3 -
drivers/gpu/drm/ingenic/ingenic-ipu.c | 8 +-
drivers/gpu/drm/mgag200/mgag200_mode.c | 3 +-
drivers/gpu/drm/sitronix/st7571.c | 3 +-
drivers/gpu/drm/sitronix/st7586.c | 3 +-
drivers/gpu/drm/sitronix/st7920.c | 3 +-
drivers/gpu/drm/solomon/ssd130x.c | 9 +-
drivers/gpu/drm/sysfb/drm_sysfb_modeset.c | 3 +-
.../gpu/drm/tests/drm_damage_helper_test.c | 200 +++---------------
drivers/gpu/drm/tiny/appletbdrm.c | 56 +++--
drivers/gpu/drm/tiny/bochs.c | 3 +-
drivers/gpu/drm/tiny/cirrus-qemu.c | 2 +-
drivers/gpu/drm/tiny/gm12u320.c | 2 +-
drivers/gpu/drm/tiny/ili9225.c | 3 +-
drivers/gpu/drm/tiny/repaper.c | 2 +-
drivers/gpu/drm/tiny/sharp-memory.c | 3 +-
drivers/gpu/drm/udl/udl_modeset.c | 3 +-
drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 5 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 2 -
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 12 +-
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 15 +-
include/drm/drm_damage_helper.h | 9 +-
34 files changed, 124 insertions(+), 311 deletions(-)
--
2.54.0
^ permalink raw reply
* Re: [PATCH net-next v10 4/4] tun/tap & vhost-net: avoid ptr_ring tail-drop when a qdisc is present
From: Simon Schippers @ 2026-05-07 6:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: willemdebruijn.kernel, jasowang, andrew+netdev, davem, edumazet,
kuba, pabeni, eperezma, leiyang, stephen, jon, tim.gebauer,
netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20260506185515-mutt-send-email-mst@kernel.org>
On 5/7/26 00:56, Michael S. Tsirkin wrote:
> On Wed, May 06, 2026 at 06:28:06PM -0400, Michael S. Tsirkin wrote:
>> On Wed, May 06, 2026 at 04:10:33PM +0200, Simon Schippers wrote:
>>> This commit prevents tail-drop when a qdisc is present and the ptr_ring
>>> becomes full. Once an entry is successfully produced and the ptr_ring
>>> reaches capacity, the netdev queue is stopped instead of dropping
>>> subsequent packets. If no qdisc is present, the previous tail-drop
>>> behavior is preserved.
>>>
>>> If producing an entry fails anyways due to a race, tun_net_xmit() drops
>>> the packet. Such races are expected because LLTX is enabled and the
>>> transmit path operates without the usual locking.
>>>
>>> The __tun_wake_queue() function of the consumer races with the producer
>>> for waking/stopping the netdev queue, which could result in a stalled
>>> queue. Therefore, an smp_mb__after_atomic() is introduced that pairs
>>> with the smp_mb() of the consumer. It follows the principle of store
>>> buffering described in tools/memory-model/Documentation/recipes.txt:
>>>
>>> - The producer in tun_net_xmit() first sets __QUEUE_STATE_DRV_XOFF,
>>> followed by an smp_mb__after_atomic() (= smp_mb()), and then reads the
>>> ring with __ptr_ring_produce_peek().
>>>
>>> - The consumer in __tun_wake_queue() first writes zero to the ring in
>>> __ptr_ring_consume(), followed by an smp_mb(), and then reads the queue
>>> status with netif_tx_queue_stopped().
>>>
>>> => Following the aforementioned principle, it is impossible for the
>>> producer to see a full ring (and therefore not wake the queue on the
>>> re-check) while the consumer simultaneously fails to see a stopped
>>> queue (and therefore also does not wake it).
>>>
>>> Benchmarks:
>>> The benchmarks show a slight regression in raw transmission performance
>>> when using two sending threads. Packet loss also occurs only in the
>>> two-thread sending case; no packet loss was observed with a single
>>> sending thread.
>>>
>>> Test setup:
>>> AMD Ryzen 5 5600X at 4.3 GHz, 3200 MHz RAM, isolated QEMU threads;
>>> Average over 50 runs @ 100,000,000 packets. SRSO and spectre v2
>>> mitigations disabled.
>>>
>>> Note for tap+vhost-net:
>>> XDP drop program active in VM -> ~2.5x faster; slower for tap due to
>>> more syscalls (high utilization of entry_SYSRETQ_unsafe_stack in perf)
>>>
>>> +--------------------------+--------------+----------------+----------+
>>> | 1 thread | Stock | Patched with | diff |
>>> | sending | | fq_codel qdisc | |
>>> +------------+-------------+--------------+----------------+----------+
>>> | TAP | Received | 1.132 Mpps | 1.133 Mpps | +0.1% |
>>> | +-------------+--------------+----------------+----------+
>>> | | Lost/s | 3.765 Mpps | 0 pps | |
>>> +------------+-------------+--------------+----------------+----------+
>>> | TAP | Received | 3.857 Mpps | 3.905 Mpps | +1.2% |
>>> | +-------------+--------------+----------------+----------+
>>> | +vhost-net | Lost/s | 0.802 Mpps | 0 pps | |
>>> +------------+-------------+--------------+----------------+----------+
>>>
>>> +--------------------------+--------------+----------------+----------+
>>> | 2 threads | Stock | Patched with | diff |
>>> | sending | | fq_codel qdisc | |
>>> +------------+-------------+--------------+----------------+----------+
>>> | TAP | Received | 1.115 Mpps | 1.092 Mpps | -2.1% |
>>> | +-------------+--------------+----------------+----------+
>>> | | Lost/s | 8.490 Mpps | 359 pps | |
>>> +------------+-------------+--------------+----------------+----------+
>>> | TAP | Received | 3.664 Mpps | 3.549 Mpps | -3.1% |
>>> | +-------------+--------------+----------------+----------+
>>> | +vhost-net | Lost/s | 5.330 Mpps | 832 pps | |
>>> +------------+-------------+--------------+----------------+----------+
>>>
>>> Co-developed-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
>>> Signed-off-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
>>> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
>>> ---
>>> drivers/net/tun.c | 25 +++++++++++++++++++++++--
>>> 1 file changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index fc358c4c355b..d9ffbf88cfd8 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>> @@ -1018,6 +1018,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>>> struct netdev_queue *queue;
>>> struct tun_file *tfile;
>>> int len = skb->len;
>>> + int ret;
>>>
>>> rcu_read_lock();
>>> tfile = rcu_dereference(tun->tfiles[txq]);
>>> @@ -1072,13 +1073,33 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>>>
>>> nf_reset_ct(skb);
>>>
>>> - if (ptr_ring_produce(&tfile->tx_ring, skb)) {
>>> + queue = netdev_get_tx_queue(dev, txq);
>>> +
>>> + spin_lock(&tfile->tx_ring.producer_lock);
>>> + ret = __ptr_ring_produce(&tfile->tx_ring, skb);
>>> + if (!qdisc_txq_has_no_queue(queue) &&
>>> + (__ptr_ring_produce_peek(&tfile->tx_ring) || ret)) {
>>> + netif_tx_stop_queue(queue);
>>> + /* Paired with smp_mb() in __tun_wake_queue() */
>>> + smp_mb__after_atomic();
>>> + if (!__ptr_ring_produce_peek(&tfile->tx_ring))
>>> + netif_tx_wake_queue(queue);
>>> + }
>>> + spin_unlock(&tfile->tx_ring.producer_lock);
>>> +
>>
>> There's a weird corner case here when tx_queue_len is 0
>> but a qdisc has been configured - it looks like that
>> currently it just drops all packets, with this change,
>> the qdisc will get stuck permanently.
>>
>> I suspect just checking tx_ring.size should fix it.
>> Or if you feel adventurous, change return code for __ptr_ring_produce
>> to distinguish between "no ring" and "no space".
>
>
> __ptr_ring_produce_peek really.
>
Yes, I like the approach of returning this from
__ptr_ring_produce_peek(). Then I will do a switch on the return value
in tun_net_xmit().
Additionally, I should wake up in tun_queue_resize() after calling
ptr_ring_resize_multiple_bh(). For a new dev->tx_queue_len > 0, it
should be fine without waking, but for 0 it is not.
>
>>
>>> + if (ret) {
>>> + /* This should be a rare case if a qdisc is present, but
>>> + * can happen due to lltx.
>>> + * Since skb_tx_timestamp(), skb_orphan(),
>>> + * run_ebpf_filter() and pskb_trim() could have tinkered
>>> + * with the SKB, returning NETDEV_TX_BUSY is unsafe and
>>> + * we must drop instead.
>>> + */
>>> drop_reason = SKB_DROP_REASON_FULL_RING;
>>> goto drop;
>>> }
>>>
>>> /* dev->lltx requires to do our own update of trans_start */
>>> - queue = netdev_get_tx_queue(dev, txq);
>>> txq_trans_cond_update(queue);
>>>
>>> /* Notify and wake up reader process */
>>> --
>>> 2.43.0
>
^ permalink raw reply
* Re: [PATCH net-next v10 1/4] tun/tap: add ptr_ring consume helper with netdev queue wakeup
From: Simon Schippers @ 2026-05-07 6:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: willemdebruijn.kernel, jasowang, andrew+netdev, davem, edumazet,
kuba, pabeni, eperezma, leiyang, stephen, jon, tim.gebauer,
netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20260506181556-mutt-send-email-mst@kernel.org>
On 5/7/26 00:18, Michael S. Tsirkin wrote:
> On Wed, May 06, 2026 at 04:10:30PM +0200, Simon Schippers wrote:
>> Introduce tun_ring_consume() that wraps ptr_ring_consume() and calls
>> __tun_wake_queue(). The latter wakes the stopped netdev subqueue once
>> half of the ring capacity has been consumed, tracked via the new
>> cons_cnt field in tun_file. cons_cnt is updated while holding the ring
>> consumer lock, avoiding races. As a safety net, the queue is also woken
>> when the ring becomes empty. The point is to allow the queue to be
>> stopped when it gets full, which is required for traffic shaping -
>> implemented by the following "avoid ptr_ring tail-drop when a qdisc
>> is present". That patch also explains the pairing of the smp_mb()
>> of __tun_wake_queue().
>>
>> Without the corresponding queue stopping, this patch alone causes no
>> regression for a tap setup sending to a qemu VM: 1.132 Mpps
>> to 1.144 Mpps.
>>
>> Details: AMD Ryzen 5 5600X at 4.3 GHz, 3200 MHz RAM, isolated QEMU
>> threads, pktgen sender; Avg over 50 runs @ 100,000,000 packets;
>> SRSO and spectre v2 mitigations disabled.
>>
>> Co-developed-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
>> Signed-off-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
>> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
>> ---
>> drivers/net/tun.c | 54 +++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 50 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index b183189f1853..00ecf128fe8e 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -145,6 +145,7 @@ struct tun_file {
>> struct list_head next;
>> struct tun_struct *detached;
>> struct ptr_ring tx_ring;
>> + int cons_cnt;
>> struct xdp_rxq_info xdp_rxq;
>> };
>>
>> @@ -557,6 +558,13 @@ void tun_ptr_free(void *ptr)
>> }
>> EXPORT_SYMBOL_GPL(tun_ptr_free);
>>
>> +static void tun_reset_cons_cnt(struct tun_file *tfile)
>> +{
>> + spin_lock(&tfile->tx_ring.consumer_lock);
>> + tfile->cons_cnt = 0;
>> + spin_unlock(&tfile->tx_ring.consumer_lock);
>> +}
>> +
>> static void tun_queue_purge(struct tun_file *tfile)
>> {
>> void *ptr;
>> @@ -564,6 +572,7 @@ static void tun_queue_purge(struct tun_file *tfile)
>> while ((ptr = ptr_ring_consume(&tfile->tx_ring)) != NULL)
>> tun_ptr_free(ptr);
>>
>> + tun_reset_cons_cnt(tfile);
>> skb_queue_purge(&tfile->sk.sk_write_queue);
>> skb_queue_purge(&tfile->sk.sk_error_queue);
>> }
>> @@ -730,6 +739,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>> goto out;
>> }
>>
>> + tun_reset_cons_cnt(tfile);
>> tfile->queue_index = tun->numqueues;
>> tfile->socket.sk->sk_shutdown &= ~RCV_SHUTDOWN;
>>
>> @@ -2115,13 +2125,46 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>> return total;
>> }
>>
>> -static void *tun_ring_recv(struct tun_file *tfile, int noblock, int *err)
>> +/* Callers must hold ring.consumer_lock */
>> +static void __tun_wake_queue(struct tun_struct *tun,
>> + struct tun_file *tfile, int consumed)
>> +{
>> + struct netdev_queue *txq = netdev_get_tx_queue(tun->dev,
>> + tfile->queue_index);
>> +
>> + /* Paired with smp_mb__after_atomic() in tun_net_xmit() */
>> + smp_mb();
>> + if (netif_tx_queue_stopped(txq)) {
>> + tfile->cons_cnt += consumed;
>> + if (tfile->cons_cnt >= tfile->tx_ring.size / 2 ||
>> + __ptr_ring_empty(&tfile->tx_ring)) {
>> + netif_tx_wake_queue(txq);
>> + tfile->cons_cnt = 0;
>> + }
>> + }
>> +}
>> +
>> +static void *tun_ring_consume(struct tun_struct *tun, struct tun_file *tfile)
>> +{
>> + void *ptr;
>> +
>> + spin_lock(&tfile->tx_ring.consumer_lock);
>> + ptr = __ptr_ring_consume(&tfile->tx_ring);
>> + if (ptr)
>> + __tun_wake_queue(tun, tfile, 1);
>> +
>> + spin_unlock(&tfile->tx_ring.consumer_lock);
>> + return ptr;
>> +}
>> +
>> +static void *tun_ring_recv(struct tun_struct *tun, struct tun_file *tfile,
>> + int noblock, int *err)
>> {
>> DECLARE_WAITQUEUE(wait, current);
>> void *ptr = NULL;
>> int error = 0;
>>
>> - ptr = ptr_ring_consume(&tfile->tx_ring);
>> + ptr = tun_ring_consume(tun, tfile);
>> if (ptr)
>> goto out;
>> if (noblock) {
>> @@ -2133,7 +2176,7 @@ static void *tun_ring_recv(struct tun_file *tfile, int noblock, int *err)
>>
>> while (1) {
>> set_current_state(TASK_INTERRUPTIBLE);
>> - ptr = ptr_ring_consume(&tfile->tx_ring);
>> + ptr = tun_ring_consume(tun, tfile);
>> if (ptr)
>> break;
>> if (signal_pending(current)) {
>
>
> So based on commit log I expected all calls to ptr_ring_consume to
> be replaced with tun_ring_consume, but it looks like tun_queue_purge
> still calls ptr_ring_consume.
> I suspect that together with patch 4 it can sometimes leave us stuck
> with a stopped queue and an empty ring, forever.
>
I see. I will replace ptr_ring_consume() with tun_ring_consume().
>
>
>
>
>> @@ -2170,7 +2213,7 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
>>
>> if (!ptr) {
>> /* Read frames from ring */
>> - ptr = tun_ring_recv(tfile, noblock, &err);
>> + ptr = tun_ring_recv(tun, tfile, noblock, &err);
>> if (!ptr)
>> return err;
>> }
>> @@ -3406,6 +3449,8 @@ static int tun_chr_open(struct inode *inode, struct file * file)
>> return -ENOMEM;
>> }
>>
>> + tun_reset_cons_cnt(tfile);
>> +
>> mutex_init(&tfile->napi_mutex);
>> RCU_INIT_POINTER(tfile->tun, NULL);
>> tfile->flags = 0;
>> @@ -3614,6 +3659,7 @@ static int tun_queue_resize(struct tun_struct *tun)
>> for (i = 0; i < tun->numqueues; i++) {
>> tfile = rtnl_dereference(tun->tfiles[i]);
>> rings[i] = &tfile->tx_ring;
>> + tun_reset_cons_cnt(tfile);
>> }
>> list_for_each_entry(tfile, &tun->disabled, next)
>> rings[i++] = &tfile->tx_ring;
>> --
>> 2.43.0
>
^ permalink raw reply
* Re: [PATCH net-next v10 4/4] tun/tap & vhost-net: avoid ptr_ring tail-drop when a qdisc is present
From: Michael S. Tsirkin @ 2026-05-06 22:56 UTC (permalink / raw)
To: Simon Schippers
Cc: willemdebruijn.kernel, jasowang, andrew+netdev, davem, edumazet,
kuba, pabeni, eperezma, leiyang, stephen, jon, tim.gebauer,
netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20260506181909-mutt-send-email-mst@kernel.org>
On Wed, May 06, 2026 at 06:28:06PM -0400, Michael S. Tsirkin wrote:
> On Wed, May 06, 2026 at 04:10:33PM +0200, Simon Schippers wrote:
> > This commit prevents tail-drop when a qdisc is present and the ptr_ring
> > becomes full. Once an entry is successfully produced and the ptr_ring
> > reaches capacity, the netdev queue is stopped instead of dropping
> > subsequent packets. If no qdisc is present, the previous tail-drop
> > behavior is preserved.
> >
> > If producing an entry fails anyways due to a race, tun_net_xmit() drops
> > the packet. Such races are expected because LLTX is enabled and the
> > transmit path operates without the usual locking.
> >
> > The __tun_wake_queue() function of the consumer races with the producer
> > for waking/stopping the netdev queue, which could result in a stalled
> > queue. Therefore, an smp_mb__after_atomic() is introduced that pairs
> > with the smp_mb() of the consumer. It follows the principle of store
> > buffering described in tools/memory-model/Documentation/recipes.txt:
> >
> > - The producer in tun_net_xmit() first sets __QUEUE_STATE_DRV_XOFF,
> > followed by an smp_mb__after_atomic() (= smp_mb()), and then reads the
> > ring with __ptr_ring_produce_peek().
> >
> > - The consumer in __tun_wake_queue() first writes zero to the ring in
> > __ptr_ring_consume(), followed by an smp_mb(), and then reads the queue
> > status with netif_tx_queue_stopped().
> >
> > => Following the aforementioned principle, it is impossible for the
> > producer to see a full ring (and therefore not wake the queue on the
> > re-check) while the consumer simultaneously fails to see a stopped
> > queue (and therefore also does not wake it).
> >
> > Benchmarks:
> > The benchmarks show a slight regression in raw transmission performance
> > when using two sending threads. Packet loss also occurs only in the
> > two-thread sending case; no packet loss was observed with a single
> > sending thread.
> >
> > Test setup:
> > AMD Ryzen 5 5600X at 4.3 GHz, 3200 MHz RAM, isolated QEMU threads;
> > Average over 50 runs @ 100,000,000 packets. SRSO and spectre v2
> > mitigations disabled.
> >
> > Note for tap+vhost-net:
> > XDP drop program active in VM -> ~2.5x faster; slower for tap due to
> > more syscalls (high utilization of entry_SYSRETQ_unsafe_stack in perf)
> >
> > +--------------------------+--------------+----------------+----------+
> > | 1 thread | Stock | Patched with | diff |
> > | sending | | fq_codel qdisc | |
> > +------------+-------------+--------------+----------------+----------+
> > | TAP | Received | 1.132 Mpps | 1.133 Mpps | +0.1% |
> > | +-------------+--------------+----------------+----------+
> > | | Lost/s | 3.765 Mpps | 0 pps | |
> > +------------+-------------+--------------+----------------+----------+
> > | TAP | Received | 3.857 Mpps | 3.905 Mpps | +1.2% |
> > | +-------------+--------------+----------------+----------+
> > | +vhost-net | Lost/s | 0.802 Mpps | 0 pps | |
> > +------------+-------------+--------------+----------------+----------+
> >
> > +--------------------------+--------------+----------------+----------+
> > | 2 threads | Stock | Patched with | diff |
> > | sending | | fq_codel qdisc | |
> > +------------+-------------+--------------+----------------+----------+
> > | TAP | Received | 1.115 Mpps | 1.092 Mpps | -2.1% |
> > | +-------------+--------------+----------------+----------+
> > | | Lost/s | 8.490 Mpps | 359 pps | |
> > +------------+-------------+--------------+----------------+----------+
> > | TAP | Received | 3.664 Mpps | 3.549 Mpps | -3.1% |
> > | +-------------+--------------+----------------+----------+
> > | +vhost-net | Lost/s | 5.330 Mpps | 832 pps | |
> > +------------+-------------+--------------+----------------+----------+
> >
> > Co-developed-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
> > Signed-off-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
> > Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
> > ---
> > drivers/net/tun.c | 25 +++++++++++++++++++++++--
> > 1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index fc358c4c355b..d9ffbf88cfd8 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1018,6 +1018,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
> > struct netdev_queue *queue;
> > struct tun_file *tfile;
> > int len = skb->len;
> > + int ret;
> >
> > rcu_read_lock();
> > tfile = rcu_dereference(tun->tfiles[txq]);
> > @@ -1072,13 +1073,33 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
> >
> > nf_reset_ct(skb);
> >
> > - if (ptr_ring_produce(&tfile->tx_ring, skb)) {
> > + queue = netdev_get_tx_queue(dev, txq);
> > +
> > + spin_lock(&tfile->tx_ring.producer_lock);
> > + ret = __ptr_ring_produce(&tfile->tx_ring, skb);
> > + if (!qdisc_txq_has_no_queue(queue) &&
> > + (__ptr_ring_produce_peek(&tfile->tx_ring) || ret)) {
> > + netif_tx_stop_queue(queue);
> > + /* Paired with smp_mb() in __tun_wake_queue() */
> > + smp_mb__after_atomic();
> > + if (!__ptr_ring_produce_peek(&tfile->tx_ring))
> > + netif_tx_wake_queue(queue);
> > + }
> > + spin_unlock(&tfile->tx_ring.producer_lock);
> > +
>
> There's a weird corner case here when tx_queue_len is 0
> but a qdisc has been configured - it looks like that
> currently it just drops all packets, with this change,
> the qdisc will get stuck permanently.
>
> I suspect just checking tx_ring.size should fix it.
> Or if you feel adventurous, change return code for __ptr_ring_produce
> to distinguish between "no ring" and "no space".
__ptr_ring_produce_peek really.
>
> > + if (ret) {
> > + /* This should be a rare case if a qdisc is present, but
> > + * can happen due to lltx.
> > + * Since skb_tx_timestamp(), skb_orphan(),
> > + * run_ebpf_filter() and pskb_trim() could have tinkered
> > + * with the SKB, returning NETDEV_TX_BUSY is unsafe and
> > + * we must drop instead.
> > + */
> > drop_reason = SKB_DROP_REASON_FULL_RING;
> > goto drop;
> > }
> >
> > /* dev->lltx requires to do our own update of trans_start */
> > - queue = netdev_get_tx_queue(dev, txq);
> > txq_trans_cond_update(queue);
> >
> > /* Notify and wake up reader process */
> > --
> > 2.43.0
^ permalink raw reply
* Re: [PATCH net-next v10 4/4] tun/tap & vhost-net: avoid ptr_ring tail-drop when a qdisc is present
From: Michael S. Tsirkin @ 2026-05-06 22:28 UTC (permalink / raw)
To: Simon Schippers
Cc: willemdebruijn.kernel, jasowang, andrew+netdev, davem, edumazet,
kuba, pabeni, eperezma, leiyang, stephen, jon, tim.gebauer,
netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20260506141033.180450-5-simon.schippers@tu-dortmund.de>
On Wed, May 06, 2026 at 04:10:33PM +0200, Simon Schippers wrote:
> This commit prevents tail-drop when a qdisc is present and the ptr_ring
> becomes full. Once an entry is successfully produced and the ptr_ring
> reaches capacity, the netdev queue is stopped instead of dropping
> subsequent packets. If no qdisc is present, the previous tail-drop
> behavior is preserved.
>
> If producing an entry fails anyways due to a race, tun_net_xmit() drops
> the packet. Such races are expected because LLTX is enabled and the
> transmit path operates without the usual locking.
>
> The __tun_wake_queue() function of the consumer races with the producer
> for waking/stopping the netdev queue, which could result in a stalled
> queue. Therefore, an smp_mb__after_atomic() is introduced that pairs
> with the smp_mb() of the consumer. It follows the principle of store
> buffering described in tools/memory-model/Documentation/recipes.txt:
>
> - The producer in tun_net_xmit() first sets __QUEUE_STATE_DRV_XOFF,
> followed by an smp_mb__after_atomic() (= smp_mb()), and then reads the
> ring with __ptr_ring_produce_peek().
>
> - The consumer in __tun_wake_queue() first writes zero to the ring in
> __ptr_ring_consume(), followed by an smp_mb(), and then reads the queue
> status with netif_tx_queue_stopped().
>
> => Following the aforementioned principle, it is impossible for the
> producer to see a full ring (and therefore not wake the queue on the
> re-check) while the consumer simultaneously fails to see a stopped
> queue (and therefore also does not wake it).
>
> Benchmarks:
> The benchmarks show a slight regression in raw transmission performance
> when using two sending threads. Packet loss also occurs only in the
> two-thread sending case; no packet loss was observed with a single
> sending thread.
>
> Test setup:
> AMD Ryzen 5 5600X at 4.3 GHz, 3200 MHz RAM, isolated QEMU threads;
> Average over 50 runs @ 100,000,000 packets. SRSO and spectre v2
> mitigations disabled.
>
> Note for tap+vhost-net:
> XDP drop program active in VM -> ~2.5x faster; slower for tap due to
> more syscalls (high utilization of entry_SYSRETQ_unsafe_stack in perf)
>
> +--------------------------+--------------+----------------+----------+
> | 1 thread | Stock | Patched with | diff |
> | sending | | fq_codel qdisc | |
> +------------+-------------+--------------+----------------+----------+
> | TAP | Received | 1.132 Mpps | 1.133 Mpps | +0.1% |
> | +-------------+--------------+----------------+----------+
> | | Lost/s | 3.765 Mpps | 0 pps | |
> +------------+-------------+--------------+----------------+----------+
> | TAP | Received | 3.857 Mpps | 3.905 Mpps | +1.2% |
> | +-------------+--------------+----------------+----------+
> | +vhost-net | Lost/s | 0.802 Mpps | 0 pps | |
> +------------+-------------+--------------+----------------+----------+
>
> +--------------------------+--------------+----------------+----------+
> | 2 threads | Stock | Patched with | diff |
> | sending | | fq_codel qdisc | |
> +------------+-------------+--------------+----------------+----------+
> | TAP | Received | 1.115 Mpps | 1.092 Mpps | -2.1% |
> | +-------------+--------------+----------------+----------+
> | | Lost/s | 8.490 Mpps | 359 pps | |
> +------------+-------------+--------------+----------------+----------+
> | TAP | Received | 3.664 Mpps | 3.549 Mpps | -3.1% |
> | +-------------+--------------+----------------+----------+
> | +vhost-net | Lost/s | 5.330 Mpps | 832 pps | |
> +------------+-------------+--------------+----------------+----------+
>
> Co-developed-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
> Signed-off-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
> ---
> drivers/net/tun.c | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index fc358c4c355b..d9ffbf88cfd8 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1018,6 +1018,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
> struct netdev_queue *queue;
> struct tun_file *tfile;
> int len = skb->len;
> + int ret;
>
> rcu_read_lock();
> tfile = rcu_dereference(tun->tfiles[txq]);
> @@ -1072,13 +1073,33 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>
> nf_reset_ct(skb);
>
> - if (ptr_ring_produce(&tfile->tx_ring, skb)) {
> + queue = netdev_get_tx_queue(dev, txq);
> +
> + spin_lock(&tfile->tx_ring.producer_lock);
> + ret = __ptr_ring_produce(&tfile->tx_ring, skb);
> + if (!qdisc_txq_has_no_queue(queue) &&
> + (__ptr_ring_produce_peek(&tfile->tx_ring) || ret)) {
> + netif_tx_stop_queue(queue);
> + /* Paired with smp_mb() in __tun_wake_queue() */
> + smp_mb__after_atomic();
> + if (!__ptr_ring_produce_peek(&tfile->tx_ring))
> + netif_tx_wake_queue(queue);
> + }
> + spin_unlock(&tfile->tx_ring.producer_lock);
> +
There's a weird corner case here when tx_queue_len is 0
but a qdisc has been configured - it looks like that
currently it just drops all packets, with this change,
the qdisc will get stuck permanently.
I suspect just checking tx_ring.size should fix it.
Or if you feel adventurous, change return code for __ptr_ring_produce
to distinguish between "no ring" and "no space".
> + if (ret) {
> + /* This should be a rare case if a qdisc is present, but
> + * can happen due to lltx.
> + * Since skb_tx_timestamp(), skb_orphan(),
> + * run_ebpf_filter() and pskb_trim() could have tinkered
> + * with the SKB, returning NETDEV_TX_BUSY is unsafe and
> + * we must drop instead.
> + */
> drop_reason = SKB_DROP_REASON_FULL_RING;
> goto drop;
> }
>
> /* dev->lltx requires to do our own update of trans_start */
> - queue = netdev_get_tx_queue(dev, txq);
> txq_trans_cond_update(queue);
>
> /* Notify and wake up reader process */
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH net-next v10 1/4] tun/tap: add ptr_ring consume helper with netdev queue wakeup
From: Michael S. Tsirkin @ 2026-05-06 22:18 UTC (permalink / raw)
To: Simon Schippers
Cc: willemdebruijn.kernel, jasowang, andrew+netdev, davem, edumazet,
kuba, pabeni, eperezma, leiyang, stephen, jon, tim.gebauer,
netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20260506141033.180450-2-simon.schippers@tu-dortmund.de>
On Wed, May 06, 2026 at 04:10:30PM +0200, Simon Schippers wrote:
> Introduce tun_ring_consume() that wraps ptr_ring_consume() and calls
> __tun_wake_queue(). The latter wakes the stopped netdev subqueue once
> half of the ring capacity has been consumed, tracked via the new
> cons_cnt field in tun_file. cons_cnt is updated while holding the ring
> consumer lock, avoiding races. As a safety net, the queue is also woken
> when the ring becomes empty. The point is to allow the queue to be
> stopped when it gets full, which is required for traffic shaping -
> implemented by the following "avoid ptr_ring tail-drop when a qdisc
> is present". That patch also explains the pairing of the smp_mb()
> of __tun_wake_queue().
>
> Without the corresponding queue stopping, this patch alone causes no
> regression for a tap setup sending to a qemu VM: 1.132 Mpps
> to 1.144 Mpps.
>
> Details: AMD Ryzen 5 5600X at 4.3 GHz, 3200 MHz RAM, isolated QEMU
> threads, pktgen sender; Avg over 50 runs @ 100,000,000 packets;
> SRSO and spectre v2 mitigations disabled.
>
> Co-developed-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
> Signed-off-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
> ---
> drivers/net/tun.c | 54 +++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index b183189f1853..00ecf128fe8e 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -145,6 +145,7 @@ struct tun_file {
> struct list_head next;
> struct tun_struct *detached;
> struct ptr_ring tx_ring;
> + int cons_cnt;
> struct xdp_rxq_info xdp_rxq;
> };
>
> @@ -557,6 +558,13 @@ void tun_ptr_free(void *ptr)
> }
> EXPORT_SYMBOL_GPL(tun_ptr_free);
>
> +static void tun_reset_cons_cnt(struct tun_file *tfile)
> +{
> + spin_lock(&tfile->tx_ring.consumer_lock);
> + tfile->cons_cnt = 0;
> + spin_unlock(&tfile->tx_ring.consumer_lock);
> +}
> +
> static void tun_queue_purge(struct tun_file *tfile)
> {
> void *ptr;
> @@ -564,6 +572,7 @@ static void tun_queue_purge(struct tun_file *tfile)
> while ((ptr = ptr_ring_consume(&tfile->tx_ring)) != NULL)
> tun_ptr_free(ptr);
>
> + tun_reset_cons_cnt(tfile);
> skb_queue_purge(&tfile->sk.sk_write_queue);
> skb_queue_purge(&tfile->sk.sk_error_queue);
> }
> @@ -730,6 +739,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
> goto out;
> }
>
> + tun_reset_cons_cnt(tfile);
> tfile->queue_index = tun->numqueues;
> tfile->socket.sk->sk_shutdown &= ~RCV_SHUTDOWN;
>
> @@ -2115,13 +2125,46 @@ static ssize_t tun_put_user(struct tun_struct *tun,
> return total;
> }
>
> -static void *tun_ring_recv(struct tun_file *tfile, int noblock, int *err)
> +/* Callers must hold ring.consumer_lock */
> +static void __tun_wake_queue(struct tun_struct *tun,
> + struct tun_file *tfile, int consumed)
> +{
> + struct netdev_queue *txq = netdev_get_tx_queue(tun->dev,
> + tfile->queue_index);
> +
> + /* Paired with smp_mb__after_atomic() in tun_net_xmit() */
> + smp_mb();
> + if (netif_tx_queue_stopped(txq)) {
> + tfile->cons_cnt += consumed;
> + if (tfile->cons_cnt >= tfile->tx_ring.size / 2 ||
> + __ptr_ring_empty(&tfile->tx_ring)) {
> + netif_tx_wake_queue(txq);
> + tfile->cons_cnt = 0;
> + }
> + }
> +}
> +
> +static void *tun_ring_consume(struct tun_struct *tun, struct tun_file *tfile)
> +{
> + void *ptr;
> +
> + spin_lock(&tfile->tx_ring.consumer_lock);
> + ptr = __ptr_ring_consume(&tfile->tx_ring);
> + if (ptr)
> + __tun_wake_queue(tun, tfile, 1);
> +
> + spin_unlock(&tfile->tx_ring.consumer_lock);
> + return ptr;
> +}
> +
> +static void *tun_ring_recv(struct tun_struct *tun, struct tun_file *tfile,
> + int noblock, int *err)
> {
> DECLARE_WAITQUEUE(wait, current);
> void *ptr = NULL;
> int error = 0;
>
> - ptr = ptr_ring_consume(&tfile->tx_ring);
> + ptr = tun_ring_consume(tun, tfile);
> if (ptr)
> goto out;
> if (noblock) {
> @@ -2133,7 +2176,7 @@ static void *tun_ring_recv(struct tun_file *tfile, int noblock, int *err)
>
> while (1) {
> set_current_state(TASK_INTERRUPTIBLE);
> - ptr = ptr_ring_consume(&tfile->tx_ring);
> + ptr = tun_ring_consume(tun, tfile);
> if (ptr)
> break;
> if (signal_pending(current)) {
So based on commit log I expected all calls to ptr_ring_consume to
be replaced with tun_ring_consume, but it looks like tun_queue_purge
still calls ptr_ring_consume.
I suspect that together with patch 4 it can sometimes leave us stuck
with a stopped queue and an empty ring, forever.
> @@ -2170,7 +2213,7 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
>
> if (!ptr) {
> /* Read frames from ring */
> - ptr = tun_ring_recv(tfile, noblock, &err);
> + ptr = tun_ring_recv(tun, tfile, noblock, &err);
> if (!ptr)
> return err;
> }
> @@ -3406,6 +3449,8 @@ static int tun_chr_open(struct inode *inode, struct file * file)
> return -ENOMEM;
> }
>
> + tun_reset_cons_cnt(tfile);
> +
> mutex_init(&tfile->napi_mutex);
> RCU_INIT_POINTER(tfile->tun, NULL);
> tfile->flags = 0;
> @@ -3614,6 +3659,7 @@ static int tun_queue_resize(struct tun_struct *tun)
> for (i = 0; i < tun->numqueues; i++) {
> tfile = rtnl_dereference(tun->tfiles[i]);
> rings[i] = &tfile->tx_ring;
> + tun_reset_cons_cnt(tfile);
> }
> list_for_each_entry(tfile, &tun->disabled, next)
> rings[i++] = &tfile->tx_ring;
> --
> 2.43.0
^ permalink raw reply
* [RFC PATCH v8 4/4] virtio-mmio: wire up noirq system sleep PM callbacks
From: Sungho Bae @ 2026-05-06 16:22 UTC (permalink / raw)
To: mst, jasowang
Cc: xuanzhuo, eperezma, virtualization, linux-kernel, Sungho Bae
In-Reply-To: <20260506162254.25576-1-baver.bae@gmail.com>
From: Sungho Bae <baver.bae@lge.com>
Add noirq system-sleep PM support to the virtio-mmio transport.
This change wires noirq freeze/restore callbacks into virtio-mmio and
hooks queue reset/reactivation into the transport config ops so virtqueues
can be reinitialized and reused across suspend/resume.
For legacy (v1) devices, keep GUEST_PAGE_SIZE programming aligned with the
noirq restore path while avoiding duplicate programming in normal restore.
This enables virtio-mmio based devices to participate safely in the noirq
PM phase, which is required for early-restore users.
Signed-off-by: Sungho Bae <baver.bae@lge.com>
---
drivers/virtio/virtio_mmio.c | 137 +++++++++++++++++++++++++----------
1 file changed, 97 insertions(+), 40 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 595c2274fbb5..4f4836103b88 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -336,6 +336,77 @@ static void vm_del_vqs(struct virtio_device *vdev)
free_irq(platform_get_irq(vm_dev->pdev, 0), vm_dev);
}
+static int vm_active_vq(struct virtio_device *vdev, struct virtqueue *vq)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ int q_num = virtqueue_get_vring_size(vq);
+
+ writel(q_num, vm_dev->base + VIRTIO_MMIO_QUEUE_NUM);
+ if (vm_dev->version == 1) {
+ u64 q_pfn = virtqueue_get_desc_addr(vq) >> PAGE_SHIFT;
+
+ /*
+ * virtio-mmio v1 uses a 32bit QUEUE PFN. If we have something
+ * that doesn't fit in 32bit, fail the setup rather than
+ * pretending to be successful.
+ */
+ if (q_pfn >> 32) {
+ dev_err(&vdev->dev,
+ "platform bug: legacy virtio-mmio must not be used with RAM above 0x%llxGB\n",
+ 0x1ULL << (32 + PAGE_SHIFT - 30));
+ return -E2BIG;
+ }
+
+ writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
+ writel(q_pfn, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+ } else {
+ u64 addr;
+
+ addr = virtqueue_get_desc_addr(vq);
+ writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
+ writel((u32)(addr >> 32),
+ vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
+
+ addr = virtqueue_get_avail_addr(vq);
+ writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
+ writel((u32)(addr >> 32),
+ vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
+
+ addr = virtqueue_get_used_addr(vq);
+ writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
+ writel((u32)(addr >> 32),
+ vm_dev->base + VIRTIO_MMIO_QUEUE_USED_HIGH);
+
+ writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
+ }
+
+ return 0;
+}
+
+static int vm_reset_vqs(struct virtio_device *vdev)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ struct virtqueue *vq;
+ int err;
+
+ virtio_device_for_each_vq(vdev, vq) {
+ /* Re-initialize vring state */
+ err = virtqueue_reinit_vring(vq);
+ if (err < 0)
+ return err;
+
+ /* Select the queue we're interested in */
+ writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
+
+ /* Activate the queue */
+ err = vm_active_vq(vdev, vq);
+ if (err < 0)
+ return err;
+ }
+
+ return 0;
+}
+
static void vm_synchronize_cbs(struct virtio_device *vdev)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
@@ -388,45 +459,9 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
vq->num_max = num;
/* Activate the queue */
- writel(virtqueue_get_vring_size(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NUM);
- if (vm_dev->version == 1) {
- u64 q_pfn = virtqueue_get_desc_addr(vq) >> PAGE_SHIFT;
-
- /*
- * virtio-mmio v1 uses a 32bit QUEUE PFN. If we have something
- * that doesn't fit in 32bit, fail the setup rather than
- * pretending to be successful.
- */
- if (q_pfn >> 32) {
- dev_err(&vdev->dev,
- "platform bug: legacy virtio-mmio must not be used with RAM above 0x%llxGB\n",
- 0x1ULL << (32 + PAGE_SHIFT - 30));
- err = -E2BIG;
- goto error_bad_pfn;
- }
-
- writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
- writel(q_pfn, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
- } else {
- u64 addr;
-
- addr = virtqueue_get_desc_addr(vq);
- writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
- writel((u32)(addr >> 32),
- vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
-
- addr = virtqueue_get_avail_addr(vq);
- writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
- writel((u32)(addr >> 32),
- vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
-
- addr = virtqueue_get_used_addr(vq);
- writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
- writel((u32)(addr >> 32),
- vm_dev->base + VIRTIO_MMIO_QUEUE_USED_HIGH);
-
- writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
- }
+ err = vm_active_vq(vdev, vq);
+ if (err < 0)
+ goto error_bad_pfn;
return vq;
@@ -528,11 +563,13 @@ static const struct virtio_config_ops virtio_mmio_config_ops = {
.reset = vm_reset,
.find_vqs = vm_find_vqs,
.del_vqs = vm_del_vqs,
+ .reset_vqs = vm_reset_vqs,
.get_features = vm_get_features,
.finalize_features = vm_finalize_features,
.bus_name = vm_bus_name,
.get_shm_region = vm_get_shm_region,
.synchronize_cbs = vm_synchronize_cbs,
+ .noirq_safe = true,
};
#ifdef CONFIG_PM_SLEEP
@@ -546,15 +583,35 @@ static int virtio_mmio_freeze(struct device *dev)
static int virtio_mmio_restore(struct device *dev)
{
struct virtio_mmio_device *vm_dev = dev_get_drvdata(dev);
+ struct virtio_driver *drv = drv_to_virtio(vm_dev->vdev.dev.driver);
- if (vm_dev->version == 1)
+ if (vm_dev->version == 1 && (!drv || !drv->restore_noirq))
writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
return virtio_device_restore(&vm_dev->vdev);
}
+static int virtio_mmio_freeze_noirq(struct device *dev)
+{
+ struct virtio_mmio_device *vm_dev = dev_get_drvdata(dev);
+
+ return virtio_device_freeze_noirq(&vm_dev->vdev);
+}
+
+static int virtio_mmio_restore_noirq(struct device *dev)
+{
+ struct virtio_mmio_device *vm_dev = dev_get_drvdata(dev);
+
+ if (vm_dev->version == 1)
+ writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
+
+ return virtio_device_restore_noirq(&vm_dev->vdev);
+}
+
static const struct dev_pm_ops virtio_mmio_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(virtio_mmio_freeze, virtio_mmio_restore)
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(virtio_mmio_freeze_noirq,
+ virtio_mmio_restore_noirq)
};
#endif
--
2.43.0
^ permalink raw reply related
* [RFC PATCH v8 3/4] virtio: add noirq system sleep PM infrastructure
From: Sungho Bae @ 2026-05-06 16:22 UTC (permalink / raw)
To: mst, jasowang
Cc: xuanzhuo, eperezma, virtualization, linux-kernel, Sungho Bae
In-Reply-To: <20260506162254.25576-1-baver.bae@gmail.com>
From: Sungho Bae <baver.bae@lge.com>
Some virtio-mmio devices, such as virtio-clock or virtio-regulator,
must become operational before the regular PM restore callback runs
because other devices may depend on them.
Add the core infrastructure needed to support noirq system-sleep PM
callbacks for virtio transports:
- virtio_add_status_noirq(): status helper without might_sleep().
- virtio_features_ok_noirq(): feature negotiation without might_sleep().
- virtio_reset_device_noirq(): device reset that skips
virtio_synchronize_cbs() (IRQ handlers are already quiesced in the
noirq phase).
- virtio_device_reinit_noirq(): full noirq bring-up sequence using the
above helpers.
- virtio_config_core_enable_noirq(): config enable with irqsave
locking.
- virtio_device_ready_noirq(): marks DRIVER_OK without
virtio_synchronize_cbs().
Not all transports can safely call reset, get_status, set_status, or
finalize_features during the noirq phase: transports like virtio-ccw
issue channel commands and wait for a completion interrupt, which will
never be delivered because device interrupts are masked at the interrupt
controller during noirq suspend/resume. To address this, introduce a
boolean field noirq_safe in struct virtio_config_ops. Transports that
implement the above operations via simple MMIO reads/writes (e.g.
virtio-mmio) set this flag; all others leave it at the default false.
The noirq helpers assert noirq_safe via WARN_ON at runtime.
virtio_device_freeze() enforces the contract at freeze time, returning
-EOPNOTSUPP early if the driver provides restore_noirq but the transport
does not meet the requirements, to prevent a deadlock on resume.
virtio_device_freeze_noirq() and virtio_device_restore_noirq() perform
secondary checks as safety nets.
Add freeze_noirq/restore_noirq callbacks to struct virtio_driver and
provide matching helper wrappers in the virtio core:
- virtio_device_freeze_noirq(): validates noirq_safe and reset_vqs
requirements, then forwards to drv->freeze_noirq().
- virtio_device_restore_noirq(): guards against unsafe transports,
runs the noirq bring-up sequence, resets existing vrings via the
new config_ops->reset_vqs() hook, then calls drv->restore_noirq().
Modify virtio_device_restore() so that when a driver provides
restore_noirq, the normal-phase restore skips the re-initialization
that was already done in the noirq phase.
Signed-off-by: Sungho Bae <baver.bae@lge.com>
---
drivers/virtio/virtio.c | 305 +++++++++++++++++++++++++++++++++-
include/linux/virtio.h | 42 +++++
include/linux/virtio_config.h | 39 +++++
3 files changed, 382 insertions(+), 4 deletions(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 98f1875f8df1..97a3ed5c2985 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -193,6 +193,17 @@ static void virtio_config_core_enable(struct virtio_device *dev)
spin_unlock_irq(&dev->config_lock);
}
+static void virtio_config_core_enable_noirq(struct virtio_device *dev)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&dev->config_lock, flags);
+ dev->config_core_enabled = true;
+ if (dev->config_change_pending)
+ __virtio_config_changed(dev);
+ spin_unlock_irqrestore(&dev->config_lock, flags);
+}
+
void virtio_add_status(struct virtio_device *dev, unsigned int status)
{
might_sleep();
@@ -200,6 +211,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
}
EXPORT_SYMBOL_GPL(virtio_add_status);
+/*
+ * Same as virtio_add_status() but without the might_sleep() assertion,
+ * so it is safe to call from noirq context.
+ *
+ * Requires the transport to have set config_ops->noirq_safe, which declares
+ * that reset, get_status, and set_status do not wait for a completion
+ * interrupt and are therefore safe during the noirq PM phase.
+ */
+void virtio_add_status_noirq(struct virtio_device *dev, unsigned int status)
+{
+ WARN_ON(!dev->config->noirq_safe);
+ dev->config->set_status(dev, dev->config->get_status(dev) | status);
+}
+EXPORT_SYMBOL_GPL(virtio_add_status_noirq);
+
/* Do some validation, then set FEATURES_OK */
static int virtio_features_ok(struct virtio_device *dev)
{
@@ -234,6 +260,32 @@ static int virtio_features_ok(struct virtio_device *dev)
return 0;
}
+/* noirq-safe variant: no might_sleep(), uses virtio_add_status_noirq() */
+static int virtio_features_ok_noirq(struct virtio_device *dev)
+{
+ unsigned int status;
+
+ /*
+ * Skip virtio_check_mem_acc_cb() here: it may sleep (e.g. Xen's
+ * xen_virtio_restricted_mem_acc() calls devm_kzalloc with GFP_KERNEL).
+ * The check was already performed during probe in virtio_features_ok();
+ * features cannot change across suspend/resume so the constraint is
+ * still satisfied.
+ */
+
+ if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
+ return 0;
+
+ virtio_add_status_noirq(dev, VIRTIO_CONFIG_S_FEATURES_OK);
+ status = dev->config->get_status(dev);
+ if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
+ dev_err(&dev->dev, "virtio: device refuses features: %x\n",
+ status);
+ return -ENODEV;
+ }
+ return 0;
+}
+
/**
* virtio_reset_device - quiesce device for removal
* @dev: the device to reset
@@ -267,6 +319,28 @@ void virtio_reset_device(struct virtio_device *dev)
}
EXPORT_SYMBOL_GPL(virtio_reset_device);
+/**
+ * virtio_reset_device_noirq - noirq-safe variant of virtio_reset_device()
+ * @dev: the device to reset
+ *
+ * Requires the transport to have set config_ops->noirq_safe.
+ */
+void virtio_reset_device_noirq(struct virtio_device *dev)
+{
+ WARN_ON(!dev->config->noirq_safe);
+
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
+ /*
+ * The noirq stage runs with device IRQ handlers disabled, so
+ * virtio_synchronize_cbs() must not be called here.
+ */
+ virtio_break_device(dev);
+#endif
+
+ dev->config->reset(dev);
+}
+EXPORT_SYMBOL_GPL(virtio_reset_device_noirq);
+
static int virtio_dev_probe(struct device *_d)
{
int err, i;
@@ -539,6 +613,7 @@ int register_virtio_device(struct virtio_device *dev)
dev->config_driver_disabled = false;
dev->config_core_enabled = false;
dev->config_change_pending = false;
+ dev->noirq_state = VIRTIO_NOIRQ_NONE;
INIT_LIST_HEAD(&dev->vqs);
spin_lock_init(&dev->vqs_list_lock);
@@ -618,7 +693,63 @@ static int virtio_device_reinit(struct virtio_device *dev)
return virtio_features_ok(dev);
}
+/*
+ * noirq-safe variant of virtio_device_reinit().
+ *
+ * Requires the transport to declare config_ops->noirq_safe, which means
+ * reset, get_status, set_status, and finalize_features are safe to call
+ * during the noirq PM phase.
+ */
+static int virtio_device_reinit_noirq(struct virtio_device *dev)
+{
+ struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
+ int ret;
+
+ /*
+ * We always start by resetting the device, in case a previous
+ * driver messed it up.
+ */
+ virtio_reset_device_noirq(dev);
+
+ /* Acknowledge that we've seen the device. */
+ virtio_add_status_noirq(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
+
+ /*
+ * Maybe driver failed before freeze.
+ * Restore the failed status, for debugging.
+ */
+ if (dev->failed)
+ virtio_add_status_noirq(dev, VIRTIO_CONFIG_S_FAILED);
+
+ if (!drv)
+ return 0;
+
+ /* We have a driver! */
+ virtio_add_status_noirq(dev, VIRTIO_CONFIG_S_DRIVER);
+
+ ret = dev->config->finalize_features(dev);
+ if (ret)
+ return ret;
+
+ return virtio_features_ok_noirq(dev);
+}
+
#ifdef CONFIG_PM_SLEEP
+static inline bool virtio_has_valid_pm_cbs(struct virtio_driver *drv)
+{
+ /* Each callback pair must be fully implemented or fully absent. */
+ bool has_freeze = drv->freeze;
+ bool has_restore = drv->restore;
+ bool has_freeze_noirq = drv->freeze_noirq;
+ bool has_restore_noirq = drv->restore_noirq;
+
+ if (has_freeze != has_restore)
+ return false;
+ if (has_freeze_noirq != has_restore_noirq)
+ return false;
+ return true;
+}
+
int virtio_device_freeze(struct virtio_device *dev)
{
struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
@@ -627,6 +758,34 @@ int virtio_device_freeze(struct virtio_device *dev)
virtio_config_core_disable(dev);
dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
+ dev->noirq_state = VIRTIO_NOIRQ_NONE;
+
+ /*
+ * freeze_noirq and restore_noirq must be implemented as a pair.
+ * freeze_noirq performs full device teardown that only
+ * restore_noirq knows how to undo, and restore_noirq assumes
+ * freeze_noirq prepared the device for re-initialization.
+ */
+ if (drv && !virtio_has_valid_pm_cbs(drv)) {
+ dev_warn(&dev->dev,
+ "freeze/restore and freeze_noirq/restore_noirq must each be paired\n");
+ virtio_config_core_enable(dev);
+ return -EINVAL;
+ }
+
+ /*
+ * If the driver provides noirq callbacks, verify that the
+ * transport supports noirq PM. The driver's freeze_noirq or
+ * restore_noirq may call transport ops (reset, get_status,
+ * set_status) that could wait for an interrupt that will never
+ * arrive if the transport is not noirq-safe.
+ */
+ if (drv && drv->restore_noirq && !dev->config->noirq_safe) {
+ dev_warn(&dev->dev,
+ "transport does not support noirq PM\n");
+ virtio_config_core_enable(dev);
+ return -EOPNOTSUPP;
+ }
if (drv && drv->freeze) {
ret = drv->freeze(dev);
@@ -645,12 +804,42 @@ int virtio_device_restore(struct virtio_device *dev)
struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
int ret;
- ret = virtio_device_reinit(dev);
- if (ret)
+ /*
+ * If the driver implements restore_noirq and the noirq phase was
+ * actually entered (freeze_noirq ran), but restore_noirq did not
+ * complete successfully, the noirq phase must have failed. PM core
+ * may continue later resume phases for global recovery, but virtio
+ * does not use the normal restore path as an implicit same-device
+ * fallback.
+ */
+ if (drv && drv->restore_noirq &&
+ dev->noirq_state == VIRTIO_NOIRQ_ENTERED) {
+ ret = -EIO;
goto err;
+ }
- if (!drv)
- return 0;
+ /*
+ * Re-initialization is needed only for drivers that do not
+ * implement restore_noirq. When restore_noirq exists, either:
+ * - NOIRQ_NONE: noirq phase was never entered, so no noirq-specific
+ * teardown occurred and the device is still live.
+ * - NOIRQ_RESTORED: noirq phase already performed reinit.
+ * (NOIRQ_ENTERED is caught above as -EIO.)
+ *
+ * Note: when a driver implements restore_noirq, freeze() must NOT
+ * perform full device teardown (e.g., must not destroy virtqueues).
+ * Full teardown is deferred to freeze_noirq(). If suspend is aborted
+ * before the noirq phase, restore() is called to undo only the
+ * partial quiesce performed by freeze(), with the device still live
+ * and in DRIVER_OK state.
+ */
+ if (!drv || !drv->restore_noirq) {
+ ret = virtio_device_reinit(dev);
+ if (ret)
+ goto err;
+ if (!drv)
+ return 0;
+ }
if (drv->restore) {
ret = drv->restore(dev);
@@ -671,6 +860,114 @@ int virtio_device_restore(struct virtio_device *dev)
return ret;
}
EXPORT_SYMBOL_GPL(virtio_device_restore);
+
+int virtio_device_freeze_noirq(struct virtio_device *dev)
+{
+ struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
+
+ if (!drv)
+ return 0;
+
+ /*
+ * Pairing is validated in virtio_device_freeze(); since both
+ * callbacks must co-exist, checking one implies the other.
+ *
+ * Verify the transport supports noirq PM. This should normally
+ * have been caught at freeze time, but guard here as well.
+ */
+ if (drv->freeze_noirq && !dev->config->noirq_safe) {
+ dev_warn(&dev->dev,
+ "transport does not support noirq PM\n");
+ return -EOPNOTSUPP;
+ }
+
+ /*
+ * If the driver provides noirq callbacks and has active vqs,
+ * the transport must support reset_vqs to restore them.
+ * Fail here so the PM core can abort the transition gracefully,
+ * rather than hitting -EOPNOTSUPP on resume.
+ */
+ if (drv->freeze_noirq && !list_empty(&dev->vqs) &&
+ !dev->config->reset_vqs) {
+ dev_warn(&dev->dev,
+ "transport does not support noirq PM restore with active vqs (missing reset_vqs)\n");
+ return -EOPNOTSUPP;
+ }
+
+ /*
+ * Invoke the driver's freeze_noirq callback and mark noirq
+ * phase entered on success. Pairing is enforced in
+ * virtio_device_freeze(), so restore_noirq also exists.
+ *
+ * If freeze_noirq fails, the driver must have rolled back to
+ * the pre-call state (per kernel PM convention), so
+ * noirq_state remains NONE to allow restore() to proceed.
+ */
+ if (drv->freeze_noirq) {
+ int ret = drv->freeze_noirq(dev);
+
+ if (!ret)
+ dev->noirq_state = VIRTIO_NOIRQ_ENTERED;
+
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(virtio_device_freeze_noirq);
+
+int virtio_device_restore_noirq(struct virtio_device *dev)
+{
+ struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
+ int ret;
+
+ if (!drv || !drv->restore_noirq)
+ return 0;
+
+ /*
+ * All transport ops called below (reset, get_status, set_status) must
+ * be noirq-safe. Return early if not - this should normally have
+ * been caught at freeze_noirq time.
+ */
+ if (!dev->config->noirq_safe) {
+ dev_warn(&dev->dev,
+ "transport does not support noirq PM; skipping restore\n");
+ return -EOPNOTSUPP;
+ }
+
+ ret = virtio_device_reinit_noirq(dev);
+ if (ret)
+ goto err;
+
+ if (!list_empty(&dev->vqs)) {
+ if (!dev->config->reset_vqs) {
+ ret = -EOPNOTSUPP;
+ goto err;
+ }
+
+ ret = dev->config->reset_vqs(dev);
+ if (ret)
+ goto err;
+ }
+
+ ret = drv->restore_noirq(dev);
+ if (ret)
+ goto err;
+
+ /* Mark that noirq restore has completed successfully. */
+ dev->noirq_state = VIRTIO_NOIRQ_RESTORED;
+
+ /* If restore_noirq set DRIVER_OK, enable config now. */
+ if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK)
+ virtio_config_core_enable_noirq(dev);
+
+ return 0;
+
+err:
+ virtio_add_status_noirq(dev, VIRTIO_CONFIG_S_FAILED);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(virtio_device_restore_noirq);
#endif
int virtio_device_reset_prepare(struct virtio_device *dev)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 3bbc4cb6a672..937bc3c56bb8 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -143,6 +143,18 @@ struct virtio_admin_cmd {
int ret;
};
+/**
+ * enum virtio_noirq_state - tracks noirq PM phase progress
+ * @VIRTIO_NOIRQ_NONE: noirq phase was not entered (only freeze ran)
+ * @VIRTIO_NOIRQ_ENTERED: freeze_noirq ran; restore_noirq is expected
+ * @VIRTIO_NOIRQ_RESTORED: restore_noirq completed successfully
+ */
+enum virtio_noirq_state {
+ VIRTIO_NOIRQ_NONE,
+ VIRTIO_NOIRQ_ENTERED,
+ VIRTIO_NOIRQ_RESTORED,
+};
+
/**
* struct virtio_device - representation of a device using virtio
* @index: unique position on the virtio bus
@@ -151,6 +163,7 @@ struct virtio_admin_cmd {
* @config_driver_disabled: configuration change reporting disabled by
* a driver
* @config_change_pending: configuration change reported while disabled
+ * @noirq_state: tracks noirq PM phase progress for restore coordination
* @config_lock: protects configuration change reporting
* @vqs_list_lock: protects @vqs.
* @dev: underlying device.
@@ -171,6 +184,7 @@ struct virtio_device {
bool config_core_enabled;
bool config_driver_disabled;
bool config_change_pending;
+ enum virtio_noirq_state noirq_state;
spinlock_t config_lock;
spinlock_t vqs_list_lock;
struct device dev;
@@ -209,8 +223,12 @@ void virtio_config_driver_enable(struct virtio_device *dev);
#ifdef CONFIG_PM_SLEEP
int virtio_device_freeze(struct virtio_device *dev);
int virtio_device_restore(struct virtio_device *dev);
+int virtio_device_freeze_noirq(struct virtio_device *dev);
+int virtio_device_restore_noirq(struct virtio_device *dev);
#endif
void virtio_reset_device(struct virtio_device *dev);
+void virtio_reset_device_noirq(struct virtio_device *dev);
+void virtio_add_status_noirq(struct virtio_device *dev, unsigned int status);
int virtio_device_reset_prepare(struct virtio_device *dev);
int virtio_device_reset_done(struct virtio_device *dev);
@@ -237,6 +255,28 @@ size_t virtio_max_dma_size(const struct virtio_device *vdev);
* changes; may be called in interrupt context.
* @freeze: optional function to call during suspend/hibernation.
* @restore: optional function to call on resume.
+ * When @restore_noirq is not implemented, core resets and reinitializes
+ * the device before calling this. When @restore_noirq succeeded, core
+ * skips reinitialization; drivers should avoid calling virtio_device_ready()
+ * if DRIVER_OK was already set in the noirq phase.
+ * When @restore_noirq failed, this callback is not invoked for same-device
+ * recovery; the saved noirq error is propagated instead.
+ * When the noirq phase was entirely skipped (e.g. suspend aborted before
+ * suspend_noirq), core skips reinitialization for drivers that implement
+ * @restore_noirq and calls @restore (if provided) to undo the freeze()
+ * quiesce. Drivers without @restore_noirq follow the normal reinit +
+ * restore path.
+ * @freeze_noirq: optional function to call during noirq suspend/hibernation.
+ * @restore_noirq: optional function to call on noirq resume.
+ * If this callback fails, PM core may still continue later resume phases
+ * for global system recovery. Virtio does not treat @restore as an
+ * implicit same-device fallback for @restore_noirq failure; drivers should
+ * only implement @restore_noirq when noirq resume is their required
+ * recovery point.
+ * A noirq restore failure is detected by the normal restore path
+ * (noirq_state == VIRTIO_NOIRQ_ENTERED, meaning freeze_noirq ran but
+ * restore_noirq did not complete) and returns -EIO instead of attempting
+ * same-device recovery.
* @reset_prepare: optional function to call when a transport specific reset
* occurs.
* @reset_done: optional function to call after transport specific reset
@@ -258,6 +298,8 @@ struct virtio_driver {
void (*config_changed)(struct virtio_device *dev);
int (*freeze)(struct virtio_device *dev);
int (*restore)(struct virtio_device *dev);
+ int (*freeze_noirq)(struct virtio_device *dev);
+ int (*restore_noirq)(struct virtio_device *dev);
int (*reset_prepare)(struct virtio_device *dev);
int (*reset_done)(struct virtio_device *dev);
void (*shutdown)(struct virtio_device *dev);
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 69f84ea85d71..0110b091f634 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -70,6 +70,9 @@ struct virtqueue_info {
* vqs_info: array of virtqueue info structures
* Returns 0 on success or error status
* @del_vqs: free virtqueues found by find_vqs().
+ * @reset_vqs: reinitialize existing virtqueues without allocating or
+ * freeing them (optional). Used during noirq restore.
+ * Returns 0 on success or error status.
* @synchronize_cbs: synchronize with the virtqueue callbacks (optional)
* The function guarantees that all memory operations on the
* queue before it are visible to the vring_interrupt() that is
@@ -108,6 +111,14 @@ struct virtqueue_info {
* Returns 0 on success or error status
* If disable_vq_and_reset is set, then enable_vq_after_reset must also be
* set.
+ * @noirq_safe: set to true if @reset, @get_status, @set_status, and
+ * @finalize_features are safe to call during the noirq phase of system
+ * suspend/resume. Transports that implement these operations via simple
+ * MMIO reads/writes (e.g. virtio-mmio) can set this flag. Transports
+ * that issue channel commands and wait for a completion interrupt (e.g.
+ * virtio-ccw) must NOT set it, because device interrupts are masked at
+ * the interrupt controller during the noirq phase, which would cause the
+ * wait to hang.
*/
struct virtio_config_ops {
void (*get)(struct virtio_device *vdev, unsigned offset,
@@ -123,6 +134,7 @@ struct virtio_config_ops {
struct virtqueue_info vqs_info[],
struct irq_affinity *desc);
void (*del_vqs)(struct virtio_device *);
+ int (*reset_vqs)(struct virtio_device *vdev);
void (*synchronize_cbs)(struct virtio_device *);
u64 (*get_features)(struct virtio_device *vdev);
void (*get_extended_features)(struct virtio_device *vdev,
@@ -137,6 +149,7 @@ struct virtio_config_ops {
struct virtio_shm_region *region, u8 id);
int (*disable_vq_and_reset)(struct virtqueue *vq);
int (*enable_vq_after_reset)(struct virtqueue *vq);
+ bool noirq_safe;
};
/**
@@ -371,6 +384,32 @@ void virtio_device_ready(struct virtio_device *dev)
dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
}
+/**
+ * virtio_device_ready_noirq - noirq-safe variant of virtio_device_ready()
+ * @dev: the virtio device
+ *
+ * Requires the transport to have set config_ops->noirq_safe, which declares
+ * that get_status and set_status do not wait for a completion interrupt.
+ */
+static inline
+void virtio_device_ready_noirq(struct virtio_device *dev)
+{
+ unsigned int status = dev->config->get_status(dev);
+
+ WARN_ON(!dev->config->noirq_safe);
+ WARN_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
+
+#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
+ /*
+ * The noirq stage runs with device IRQ handlers disabled, so
+ * virtio_synchronize_cbs() must not be called here.
+ */
+ __virtio_unbreak_device(dev);
+#endif
+
+ dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
+}
+
static inline
const char *virtio_bus_name(struct virtio_device *vdev)
{
--
2.43.0
^ permalink raw reply related
* [RFC PATCH v8 2/4] virtio_ring: export virtqueue_reinit_vring() for noirq restore
From: Sungho Bae @ 2026-05-06 16:22 UTC (permalink / raw)
To: mst, jasowang
Cc: xuanzhuo, eperezma, virtualization, linux-kernel, Sungho Bae
In-Reply-To: <20260506162254.25576-1-baver.bae@gmail.com>
From: Sungho Bae <baver.bae@lge.com>
After a device reset in noirq context the existing vrings must be
re-initialized without any memory allocation, because GFP_KERNEL is
not available.
The internal helpers virtqueue_reset_split() and
virtqueue_reset_packed() already reset vring indices and descriptor
state in place. Add a thin exported wrapper, virtqueue_reinit_vring(),
that dispatches to the appropriate helper based on the ring layout.
This will be used by a subsequent patch that adds noirq system-sleep
PM callbacks for virtio-mmio.
Signed-off-by: Sungho Bae <baver.bae@lge.com>
---
drivers/virtio/virtio_ring.c | 58 ++++++++++++++++++++++++++++++++++++
include/linux/virtio_ring.h | 3 ++
2 files changed, 61 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fbca7ce1c6bf..d3339b820f6b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -506,6 +506,15 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
vq->event_triggered = false;
vq->num_added = 0;
+ /*
+ * Keep IN_ORDER state aligned with a freshly initialized/reset queue.
+ * For packed IN_ORDER, free_head is unused but harmlessly reset.
+ */
+ if (virtqueue_is_in_order(vq)) {
+ vq->free_head = 0;
+ vq->batch_last.id = UINT_MAX;
+ }
+
#ifdef DEBUG
vq->in_use = false;
vq->last_add_time_valid = false;
@@ -3936,5 +3945,54 @@ void virtqueue_map_sync_single_range_for_device(const struct virtqueue *_vq,
}
EXPORT_SYMBOL_GPL(virtqueue_map_sync_single_range_for_device);
+/**
+ * virtqueue_reinit_vring - reinitialize vring state without reallocation
+ * @_vq: the virtqueue
+ *
+ * Reset the avail/used indices and descriptor state of an existing
+ * virtqueue so it can be reused after a device reset. No memory is
+ * allocated or freed, making this safe for use in noirq context.
+ *
+ * Preconditions for callers:
+ * 1) The vq must be fully quiesced (no concurrent add/get/kick/IRQ callback).
+ * 2) Transport/device side must already have stopped/reset this queue.
+ * 3) All in-flight buffers must already be completed or detached.
+ *
+ * If called with outstanding descriptors, free-list state can be corrupted:
+ * num_free is restored to full capacity while desc_extra next-chain/free_head
+ * may still represent a partially consumed list.
+ *
+ * Return:
+ * 0 on success, or -EBUSY if preconditions are not met.
+ */
+int virtqueue_reinit_vring(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ unsigned int num = virtqueue_is_packed(vq) ?
+ vq->packed.vring.num : vq->split.vring.num;
+
+ /* All in-flight descriptors must be completed or detached */
+ if (WARN_ON(vq->vq.num_free != num))
+ return -EBUSY;
+
+ if (virtqueue_is_packed(vq)) {
+ virtqueue_reset_packed(vq);
+ } else {
+ /*
+ * Split queue shadow index should match the visible avail
+ * index when the queue is fully quiesced.
+ */
+ if (WARN_ON(vq->split.avail_idx_shadow !=
+ virtio16_to_cpu(vq->vq.vdev,
+ vq->split.vring.avail->idx)))
+ return -EBUSY;
+
+ virtqueue_reset_split(vq);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(virtqueue_reinit_vring);
+
MODULE_DESCRIPTION("Virtio ring implementation");
MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index c97a12c1cda3..8b421fef4fef 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -118,6 +118,9 @@ void vring_del_virtqueue(struct virtqueue *vq);
/* Filter out transport-specific feature bits. */
void vring_transport_features(struct virtio_device *vdev);
+/* Reinitialize a virtqueue without reallocation (safe in noirq context) */
+int virtqueue_reinit_vring(struct virtqueue *_vq);
+
irqreturn_t vring_interrupt(int irq, void *_vq);
u32 vring_notification_data(struct virtqueue *_vq);
--
2.43.0
^ permalink raw reply related
* [RFC PATCH v8 1/4] virtio: separate PM restore and reset_done paths
From: Sungho Bae @ 2026-05-06 16:22 UTC (permalink / raw)
To: mst, jasowang
Cc: xuanzhuo, eperezma, virtualization, linux-kernel, Sungho Bae
In-Reply-To: <20260506162254.25576-1-baver.bae@gmail.com>
From: Sungho Bae <baver.bae@lge.com>
Refactor virtio_device_restore_priv() by extracting the common device
re-initialization sequence into virtio_device_reinit(). This helper
performs the full bring-up sequence: reset, status acknowledgment,
feature finalization, and feature negotiation.
virtio_device_restore() and virtio_device_reset_done() now each call
virtio_device_reinit() directly instead of going through a boolean-
dispatched wrapper. This makes each path independently readable and
extensible without further complicating the dispatch logic.
A follow-up series will add noirq PM callbacks that only affect the
restore path; having the two paths separated avoids adding more
conditionals to a shared function.
No functional change.
Signed-off-by: Sungho Bae <baver.bae@lge.com>
---
drivers/virtio/virtio.c | 81 +++++++++++++++++++++++++----------------
1 file changed, 50 insertions(+), 31 deletions(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 5bdc6b82b30b..98f1875f8df1 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -588,7 +588,7 @@ void unregister_virtio_device(struct virtio_device *dev)
}
EXPORT_SYMBOL_GPL(unregister_virtio_device);
-static int virtio_device_restore_priv(struct virtio_device *dev, bool restore)
+static int virtio_device_reinit(struct virtio_device *dev)
{
struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
int ret;
@@ -613,35 +613,9 @@ static int virtio_device_restore_priv(struct virtio_device *dev, bool restore)
ret = dev->config->finalize_features(dev);
if (ret)
- goto err;
-
- ret = virtio_features_ok(dev);
- if (ret)
- goto err;
-
- if (restore) {
- if (drv->restore) {
- ret = drv->restore(dev);
- if (ret)
- goto err;
- }
- } else {
- ret = drv->reset_done(dev);
- if (ret)
- goto err;
- }
-
- /* If restore didn't do it, mark device DRIVER_OK ourselves. */
- if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
- virtio_device_ready(dev);
-
- virtio_config_core_enable(dev);
-
- return 0;
+ return ret;
-err:
- virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
- return ret;
+ return virtio_features_ok(dev);
}
#ifdef CONFIG_PM_SLEEP
@@ -668,7 +642,33 @@ EXPORT_SYMBOL_GPL(virtio_device_freeze);
int virtio_device_restore(struct virtio_device *dev)
{
- return virtio_device_restore_priv(dev, true);
+ struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
+ int ret;
+
+ ret = virtio_device_reinit(dev);
+ if (ret)
+ goto err;
+
+ if (!drv)
+ return 0;
+
+ if (drv->restore) {
+ ret = drv->restore(dev);
+ if (ret)
+ goto err;
+ }
+
+ /* If restore didn't do it, mark device DRIVER_OK ourselves. */
+ if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
+ virtio_device_ready(dev);
+
+ virtio_config_core_enable(dev);
+
+ return 0;
+
+err:
+ virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
+ return ret;
}
EXPORT_SYMBOL_GPL(virtio_device_restore);
#endif
@@ -698,11 +698,30 @@ EXPORT_SYMBOL_GPL(virtio_device_reset_prepare);
int virtio_device_reset_done(struct virtio_device *dev)
{
struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
+ int ret;
if (!drv || !drv->reset_done)
return -EOPNOTSUPP;
- return virtio_device_restore_priv(dev, false);
+ ret = virtio_device_reinit(dev);
+ if (ret)
+ goto err;
+
+ ret = drv->reset_done(dev);
+ if (ret)
+ goto err;
+
+ /* If reset_done didn't do it, mark device DRIVER_OK ourselves. */
+ if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
+ virtio_device_ready(dev);
+
+ virtio_config_core_enable(dev);
+
+ return 0;
+
+err:
+ virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
+ return ret;
}
EXPORT_SYMBOL_GPL(virtio_device_reset_done);
--
2.43.0
^ permalink raw reply related
* [RFC PATCH v8 0/4] virtio: add noirq system sleep PM callbacks for virtio-mmio
From: Sungho Bae @ 2026-05-06 16:22 UTC (permalink / raw)
To: mst, jasowang
Cc: xuanzhuo, eperezma, virtualization, linux-kernel, Sungho Bae
From: Sungho Bae <baver.bae@lge.com>
Hi all,
Some virtio-mmio based devices, such as virtio-clock or virtio-regulator,
must become operational before other devices have their regular PM restore
callbacks invoked, because those other devices depend on them.
Generally, PM framework provides the three phases (freeze, freeze_late,
freeze_noirq) for the system sleep sequence, and the corresponding resume
phases. But, virtio core only supports the normal freeze/restore phase,
so virtio drivers have no way to participate in the noirq phase, which runs
with IRQs disabled and is guaranteed to run before any normal-phase restore
callbacks.
This series adds the infrastructure and the virtio-mmio transport
wiring so that virtio drivers can implement freeze_noirq/restore_noirq
callbacks.
Design overview
===============
The noirq phase runs with device IRQ handlers disabled and must avoid
sleepable operations. The main constraints addressed are:
- might_sleep() in virtio_add_status() and virtio_features_ok().
- virtio_synchronize_cbs() in virtio_reset_device() (IRQs are already
quiesced).
- spin_lock_irq() in virtio_config_core_enable() (not safe to call
with interrupts already disabled).
- Memory allocation during vq setup (virtqueue_reinit_vring() reuses
existing buffers instead).
The series provides noirq-safe variants for each of these, plus a new
config_ops->reset_vqs() callback that lets the transport reprogram
queue registers without freeing/reallocating vring memory.
Not all transports can safely perform these operations in the noirq phase.
Transports like virtio-ccw issue channel commands and wait for a completion
interrupt, which will never arrive while device interrupts are masked at
the interrupt controller. A new boolean field config_ops->noirq_safe marks
transports that implement reset/status operations via simple MMIO
reads/writes and are therefore safe to use in noirq context. The noirq
helpers assert this flag at runtime, and virtio_device_freeze_noirq()
enforces it at freeze time, returning -EOPNOTSUPP early to prevent
a deadlock on resume.
When a driver implements restore_noirq, the device bring-up (reset ->
ACKNOWLEDGE -> DRIVER -> finalize_features -> FEATURES_OK) happens in
the noirq phase. The subsequent normal-phase virtio_device_restore()
detects this and skips the redundant re-initialization.
Patch breakdown
===============
Patch 1 is a preparatory refactoring with no functional change.
Patches 2-3 add the core infrastructure. Patch 4 wires it up for
virtio-mmio.
1. virtio: separate PM restore and reset_done paths
Splits virtio_device_restore_priv() into independent
virtio_device_restore() and virtio_device_reset_done() paths,
using a shared virtio_device_reinit() helper. This is a pure
refactoring to make the restore path independently extensible
without complicating the boolean dispatch.
2. virtio_ring: export virtqueue_reinit_vring() for noirq restore
Adds virtqueue_reinit_vring(), an exported wrapper that resets
vring indices and descriptor state in place without any memory
allocation, making it safe to call from noirq context. Also
resets IN_ORDER-specific state (free_head, batch_last.id) in
virtqueue_init() to keep the ring consistent after reinit, and
adds runtime WARN_ON checks for unexpected in-flight descriptor
state and split-ring index consistency.
3. virtio: add noirq system sleep PM infrastructure
Adds noirq-safe helpers (virtio_add_status_noirq,
virtio_features_ok_noirq, virtio_reset_device_noirq,
virtio_config_core_enable_noirq, virtio_device_ready_noirq) and
the freeze_noirq/restore_noirq driver callbacks plus the
config_ops->reset_vqs() transport hook. Introduces
config_ops->noirq_safe to mark transports whose reset/status
operations are safe in noirq context (e.g. simple MMIO), and
enforces early -EOPNOTSUPP in virtio_device_freeze() when
the transport does not meet noirq requirements. Modifies
virtio_device_restore() to skip bring-up when restore_noirq
already ran.
4. virtio-mmio: wire up noirq system sleep PM callbacks
Implements vm_reset_vqs() which iterates existing virtqueues,
reinitializes the vring state via virtqueue_reinit_vring(), and
reprograms the MMIO queue registers. Adds
virtio_mmio_freeze_noirq/virtio_mmio_restore_noirq and registers
them via SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(). Sets .noirq_safe = true
in virtio_mmio_config_ops to declare that MMIO-based status and
reset operations are safe during the noirq PM phase.
Testing
=======
Build-tested with arm64 cross-compilation.
(make ARCH=arm64 M=drivers/virtio)
Runtime-tested on an internal virtio-mmio platform with virtio-clock,
confirming that the clock device works well before other devices' normal
restore() callbacks run.
Changes
=======
v8:
virtio: add noirq system sleep PM infrastructure
- Enforced freeze/restore and freeze_noirq/restore_noirq callback
pairing in virtio_device_freeze() via virtio_has_valid_pm_cbs()
- Skipped virtio_check_mem_acc_cb() in noirq path (may sleep)
- set VIRTIO_NOIRQ_ENTERED only on freeze_noirq success to allow
restore() to proceed on failure.
v7:
virtio: add noirq system sleep PM infrastructure
- Configured virtio_noirq_state to have 3 states to differentiate
between restore_noirq failures and skipping the noirq phase.
- Re-verified the PM callback combinations.
virtio-mmio: wire up noirq system sleep PM callbacks
- Aligned both conditions for GUEST_PAGE_SIZE and virtio_device_reinit()
v6:
virtio_ring: export virtqueue_reinit_vring() for noirq restore
- Made virtqueue_reinit_vring() fail with -EBUSY on precondition
violations and propagate that error.
virtio: add noirq system sleep PM infrastructure
- Make noirq restore failure terminal for the same device:
.restore is not a same-device fallback for .restore_noirq failure.
- Decouple noirq failure from pre-freeze dev->failed snapshot.
- Add noirq_safe validation in virtio_device_freeze() to catch transport
mismatch early.
virtio-mmio: wire up noirq system sleep PM callbacks
- Make vm_reset_vqs() handle virtqueue_reinit_vring() failures
to prevent continuing noirq restore with a potentially corrupted
split free-list state.
v5:
virtio: add noirq system sleep PM infrastructure
- Preserve FAILED across restore_noirq() failure by recording the
failure in dev->failed before falling back to the normal restore path.
- Document the restore/restore_noirq fallback contract more clearly,
especially for drivers that preserve virtqueues across suspend.
v4:
virtio_ring: export virtqueue_reinit_vring() for noirq restore
- Reinit safety was tightened by resetting IN_ORDER-specific state.
- Added extra split-ring consistency WARN_ON checks.
- Clarified caller preconditions for noirq-safe vring reinit.
virtio: add noirq system sleep PM infrastructure
- Added config_ops->noirq_safe to explicitly mark transports that are
safe in noirq PM phase.
- Enforced early -EOPNOTSUPP checks in freeze_noirq for unsupported
transport combinations (noirq_safe/reset_vqs requirements).
- Added defensive runtime guards/warnings in noirq helper and restore
paths.
- Discussed the freeze-before-freeze_noirq abort scenario raised in
review and concluded that the fallback restore path is intentionally
handled by regular restore after core reinit/reset, so reset_vqs is
kept limited to the noirq restore flow.
virtio-mmio: wire up noirq system sleep PM callbacks
- Marked virtio-mmio transport as noirq-capable by setting .noirq_safe
in virtio_mmio_config_ops.
v3:
virtio: separate PM restore and reset_done paths
- Refined restore flow to explicitly handle the no-driver case after
reinit, improving clarity and avoiding unnecessary driver-path
assumptions.
virtio_ring: export virtqueue_reinit_vring() for noirq restore
- Hardened virtqueue_reinit_vring() with stronger safety notes and
a runtime WARN_ON check to catch reinit with unexpected free-list
state.
virtio: add noirq system sleep PM infrastructure
- Added explicit noirq restore completion tracking noirq_restore_done
and updated PM sequencing to use it, plus early freeze_noirq
validation for missing reset_vqs support.
virtio-mmio: wire up noirq system sleep PM callbacks
- Updated virtio-mmio restore path to skip legacy GUEST_PAGE_SIZE
rewrite when noirq restore already completed.
v2:
virtio-mmio: wire up noirq system sleep PM callbacks
- The code that was duplicated in vm_setup_vq() and vm_reset_vqs() has
been moved to vm_active_vq() function.
Sungho Bae (4):
virtio: separate PM restore and reset_done paths
virtio_ring: export virtqueue_reinit_vring() for noirq restore
virtio: add noirq system sleep PM infrastructure
virtio-mmio: wire up noirq system sleep PM callbacks
drivers/virtio/virtio.c | 368 +++++++++++++++++++++++++++++++---
drivers/virtio/virtio_mmio.c | 137 +++++++++----
drivers/virtio/virtio_ring.c | 58 ++++++
include/linux/virtio.h | 42 ++++
include/linux/virtio_config.h | 39 ++++
include/linux/virtio_ring.h | 3 +
6 files changed, 581 insertions(+), 66 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [PATCH net] vsock/virtio: fix potential unbounded skb queue
From: Michael S. Tsirkin @ 2026-05-06 15:49 UTC (permalink / raw)
To: Eric Dumazet
Cc: Stefano Garzarella, David S . Miller, Jakub Kicinski, Paolo Abeni,
Simon Horman, netdev, eric.dumazet, Arseniy Krasnov,
Stefan Hajnoczi, Jason Wang, Xuan Zhuo, Eugenio Pérez, kvm,
virtualization
In-Reply-To: <CANn89iKWhJp2N-60FC4JpX2Dw0sK8-vk2WrHkDOnkgC_c8nbHA@mail.gmail.com>
On Wed, May 06, 2026 at 08:38:39AM -0700, Eric Dumazet wrote:
> On Wed, May 6, 2026 at 8:15 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, May 05, 2026 at 07:14:36AM -0700, Eric Dumazet wrote:
> > > There is always a discrepancy between skb->len and skb->truesize.
> > > You will not be able to announce a 1MB window, and accept one milliion
> > > skb of 1-byte each.
> >
> > We can if we copy.
>
> You mean, ignore VIRTIO_VSOCK_SEQ_EOM?
No I mean saving it in a compact form.
If packet boundaries are the only concern, the overhead
needn't exceed 50% even for 1 byte messages. asn.1 ber, for
an over-engineered example?
^ permalink raw reply
* Re: [PATCH net] vsock/virtio: fix potential unbounded skb queue
From: Eric Dumazet @ 2026-05-06 15:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefano Garzarella, David S . Miller, Jakub Kicinski, Paolo Abeni,
Simon Horman, netdev, eric.dumazet, Arseniy Krasnov,
Stefan Hajnoczi, Jason Wang, Xuan Zhuo, Eugenio Pérez, kvm,
virtualization
In-Reply-To: <20260506111507-mutt-send-email-mst@kernel.org>
On Wed, May 6, 2026 at 8:15 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, May 05, 2026 at 07:14:36AM -0700, Eric Dumazet wrote:
> > There is always a discrepancy between skb->len and skb->truesize.
> > You will not be able to announce a 1MB window, and accept one milliion
> > skb of 1-byte each.
>
> We can if we copy.
You mean, ignore VIRTIO_VSOCK_SEQ_EOM?
^ permalink raw reply
* Re: [PATCH net] vsock/virtio: fix potential unbounded skb queue
From: Michael S. Tsirkin @ 2026-05-06 15:37 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Eric Dumazet, Arseniy Krasnov, Bobby Eshleman, Stefan Hajnoczi,
David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
netdev, eric.dumazet, Arseniy Krasnov, Jason Wang, Xuan Zhuo,
Eugenio Pérez, kvm, virtualization
In-Reply-To: <afoF_cHfl6ygcupM@sgarzare-redhat>
On Tue, May 05, 2026 at 06:11:13PM +0200, Stefano Garzarella wrote:
> On Tue, May 05, 2026 at 07:14:36AM -0700, Eric Dumazet wrote:
> > On Tue, May 5, 2026 at 6:52 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >
> > > On Thu, Apr 30, 2026 at 12:26:52PM +0000, Eric Dumazet wrote:
> > > >virtio_transport_inc_rx_pkt() checks vvs->rx_bytes + len > vvs->buf_alloc.
> > > >
> > > >virtio_transport_recv_enqueue() skips coalescing for packets
> > > >with VIRTIO_VSOCK_SEQ_EOM.
> > > >
> > > >If fed with packets with len == 0 and VIRTIO_VSOCK_SEQ_EOM,
> > > >a very large number of packets can be queued
> > > >because vvs->rx_bytes stays at 0.
> > > >
> > > >Fix this by estimating the skb metadata size:
> > > >
> > > > (Number of skbs in the queue) * SKB_TRUESIZE(0)
> > > >
> > > >Fixes: 077706165717 ("virtio/vsock: don't use skbuff state to account credit")
> > > >Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > >Cc: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> > > >Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > >Cc: Stefano Garzarella <sgarzare@redhat.com>
> > > >Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > >Cc: Jason Wang <jasowang@redhat.com>
> > > >Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > >Cc: "Eugenio Pérez" <eperezma@redhat.com>
> > > >Cc: kvm@vger.kernel.org
> > > >Cc: virtualization@lists.linux.dev
> > > >---
> > > > net/vmw_vsock/virtio_transport_common.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > >diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > > >index 416d533f493d7b07e9c77c43f741d28cfcd0953e..9b8014516f4fb1130ae184635fbba4dfee58bd64 100644
> > > >--- a/net/vmw_vsock/virtio_transport_common.c
> > > >+++ b/net/vmw_vsock/virtio_transport_common.c
> > > >@@ -447,7 +447,9 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
> > > > static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
> > > > u32 len)
> > > > {
> > > >- if (vvs->buf_used + len > vvs->buf_alloc)
> > > >+ u64 skb_overhead = (skb_queue_len(&vvs->rx_queue) + 1) * SKB_TRUESIZE(0);
> > > >+
> > > >+ if (skb_overhead + vvs->buf_used + len > vvs->buf_alloc)
> > > > return false;
> > >
> > > I'm not sure about this fix, I mean that maybe this is incomplete.
> > > In virtio-vsock, there is a credit mechanism between the two peers:
> > > https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-4850003
> > >
> > > This takes only the payload into account, so it’s true that this problem
> > > exists; however, perhaps we should also inform the other peer of a lower
> > > credit balance, otherwise the other peer will believe it has much more
> > > credit than it actually does, send a large payload, and then the packet
> > > will be discarded and the data lost (there are no retransmissions,
> > > etc.).
> >
> > I dunno, perhaps revert 077706165717 ("virtio/vsock: don't use skbuff
> > state to account credit")
> > and find a better fix then?
>
> IIRC the same issue was there before the commit fixed by that one (commit
> 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")), so
> not sure about reverting it TBH.
>
> CCing Arseniy and Bobby.
>
> >
> > There is always a discrepancy between skb->len and skb->truesize.
> > You will not be able to announce a 1MB window, and accept one milliion
> > skb of 1-byte each.
> >
> > This kind of contract is broken.
> >
>
> Yep, I agree, but before we start discarding data (and losing it), IMHO we
> should at least inform the other peer that we're out of space.
>
> @Stefan, @Michael, do you think we can do something in the spec to avoid
> this issue and in some way take into account also the metadata in the
> credit. I mean to avoid the 1-byte packets flooding.
>
> Thanks,
> Stefano
Why do we need the metadata? Just don't keep it around if you begin
running low on memory.
--
MST
^ permalink raw reply
* Re: [PATCH net] vsock/virtio: fix potential unbounded skb queue
From: Michael S. Tsirkin @ 2026-05-06 15:15 UTC (permalink / raw)
To: Eric Dumazet
Cc: Stefano Garzarella, David S . Miller, Jakub Kicinski, Paolo Abeni,
Simon Horman, netdev, eric.dumazet, Arseniy Krasnov,
Stefan Hajnoczi, Jason Wang, Xuan Zhuo, Eugenio Pérez, kvm,
virtualization
In-Reply-To: <CANn89iLs8DOWJwDpf_ARoMrV+6b2tbhEJ=VVzeC8gCm5dRGaig@mail.gmail.com>
On Tue, May 05, 2026 at 07:14:36AM -0700, Eric Dumazet wrote:
> There is always a discrepancy between skb->len and skb->truesize.
> You will not be able to announce a 1MB window, and accept one milliion
> skb of 1-byte each.
We can if we copy.
> This kind of contract is broken.
^ permalink raw reply
* [PATCH net-next v10 3/4] ptr_ring: move free-space check into separate helper
From: Simon Schippers @ 2026-05-06 14:10 UTC (permalink / raw)
To: willemdebruijn.kernel, jasowang, andrew+netdev, davem, edumazet,
kuba, pabeni, mst, eperezma, leiyang, stephen, jon, tim.gebauer,
simon.schippers, netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20260506141033.180450-1-simon.schippers@tu-dortmund.de>
This patch moves the check for available free space for a new entry into
a separate function. As a result, __ptr_ring_produce() remains logically
unchanged, while the new helper allows callers to determine in advance
whether subsequent __ptr_ring_produce() calls will succeed. This
information can, for example, be used to temporarily stop producing until
__ptr_ring_produce_peek() indicates that space is available again.
Co-developed-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
Signed-off-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
---
include/linux/ptr_ring.h | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index d2c3629bbe45..0887284e5b43 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -96,6 +96,17 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r)
return ret;
}
+/* Note: callers invoking this in a loop must use a compiler barrier,
+ * for example cpu_relax(). Callers must hold producer_lock.
+ */
+static inline int __ptr_ring_produce_peek(struct ptr_ring *r)
+{
+ if (unlikely(!r->size) || data_race(r->queue[r->producer]))
+ return -ENOSPC;
+
+ return 0;
+}
+
/* Note: callers invoking this in a loop must use a compiler barrier,
* for example cpu_relax(). Callers must hold producer_lock.
* Callers are responsible for making sure pointer that is being queued
@@ -103,8 +114,10 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r)
*/
static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
{
- if (unlikely(!r->size) || data_race(r->queue[r->producer]))
- return -ENOSPC;
+ int p = __ptr_ring_produce_peek(r);
+
+ if (p)
+ return p;
/* Make sure the pointer we are storing points to a valid data. */
/* Pairs with the dependency ordering in __ptr_ring_consume. */
--
2.43.0
^ permalink raw reply related
* [PATCH net-next v10 4/4] tun/tap & vhost-net: avoid ptr_ring tail-drop when a qdisc is present
From: Simon Schippers @ 2026-05-06 14:10 UTC (permalink / raw)
To: willemdebruijn.kernel, jasowang, andrew+netdev, davem, edumazet,
kuba, pabeni, mst, eperezma, leiyang, stephen, jon, tim.gebauer,
simon.schippers, netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20260506141033.180450-1-simon.schippers@tu-dortmund.de>
This commit prevents tail-drop when a qdisc is present and the ptr_ring
becomes full. Once an entry is successfully produced and the ptr_ring
reaches capacity, the netdev queue is stopped instead of dropping
subsequent packets. If no qdisc is present, the previous tail-drop
behavior is preserved.
If producing an entry fails anyways due to a race, tun_net_xmit() drops
the packet. Such races are expected because LLTX is enabled and the
transmit path operates without the usual locking.
The __tun_wake_queue() function of the consumer races with the producer
for waking/stopping the netdev queue, which could result in a stalled
queue. Therefore, an smp_mb__after_atomic() is introduced that pairs
with the smp_mb() of the consumer. It follows the principle of store
buffering described in tools/memory-model/Documentation/recipes.txt:
- The producer in tun_net_xmit() first sets __QUEUE_STATE_DRV_XOFF,
followed by an smp_mb__after_atomic() (= smp_mb()), and then reads the
ring with __ptr_ring_produce_peek().
- The consumer in __tun_wake_queue() first writes zero to the ring in
__ptr_ring_consume(), followed by an smp_mb(), and then reads the queue
status with netif_tx_queue_stopped().
=> Following the aforementioned principle, it is impossible for the
producer to see a full ring (and therefore not wake the queue on the
re-check) while the consumer simultaneously fails to see a stopped
queue (and therefore also does not wake it).
Benchmarks:
The benchmarks show a slight regression in raw transmission performance
when using two sending threads. Packet loss also occurs only in the
two-thread sending case; no packet loss was observed with a single
sending thread.
Test setup:
AMD Ryzen 5 5600X at 4.3 GHz, 3200 MHz RAM, isolated QEMU threads;
Average over 50 runs @ 100,000,000 packets. SRSO and spectre v2
mitigations disabled.
Note for tap+vhost-net:
XDP drop program active in VM -> ~2.5x faster; slower for tap due to
more syscalls (high utilization of entry_SYSRETQ_unsafe_stack in perf)
+--------------------------+--------------+----------------+----------+
| 1 thread | Stock | Patched with | diff |
| sending | | fq_codel qdisc | |
+------------+-------------+--------------+----------------+----------+
| TAP | Received | 1.132 Mpps | 1.133 Mpps | +0.1% |
| +-------------+--------------+----------------+----------+
| | Lost/s | 3.765 Mpps | 0 pps | |
+------------+-------------+--------------+----------------+----------+
| TAP | Received | 3.857 Mpps | 3.905 Mpps | +1.2% |
| +-------------+--------------+----------------+----------+
| +vhost-net | Lost/s | 0.802 Mpps | 0 pps | |
+------------+-------------+--------------+----------------+----------+
+--------------------------+--------------+----------------+----------+
| 2 threads | Stock | Patched with | diff |
| sending | | fq_codel qdisc | |
+------------+-------------+--------------+----------------+----------+
| TAP | Received | 1.115 Mpps | 1.092 Mpps | -2.1% |
| +-------------+--------------+----------------+----------+
| | Lost/s | 8.490 Mpps | 359 pps | |
+------------+-------------+--------------+----------------+----------+
| TAP | Received | 3.664 Mpps | 3.549 Mpps | -3.1% |
| +-------------+--------------+----------------+----------+
| +vhost-net | Lost/s | 5.330 Mpps | 832 pps | |
+------------+-------------+--------------+----------------+----------+
Co-developed-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
Signed-off-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
---
drivers/net/tun.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index fc358c4c355b..d9ffbf88cfd8 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1018,6 +1018,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
struct netdev_queue *queue;
struct tun_file *tfile;
int len = skb->len;
+ int ret;
rcu_read_lock();
tfile = rcu_dereference(tun->tfiles[txq]);
@@ -1072,13 +1073,33 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
nf_reset_ct(skb);
- if (ptr_ring_produce(&tfile->tx_ring, skb)) {
+ queue = netdev_get_tx_queue(dev, txq);
+
+ spin_lock(&tfile->tx_ring.producer_lock);
+ ret = __ptr_ring_produce(&tfile->tx_ring, skb);
+ if (!qdisc_txq_has_no_queue(queue) &&
+ (__ptr_ring_produce_peek(&tfile->tx_ring) || ret)) {
+ netif_tx_stop_queue(queue);
+ /* Paired with smp_mb() in __tun_wake_queue() */
+ smp_mb__after_atomic();
+ if (!__ptr_ring_produce_peek(&tfile->tx_ring))
+ netif_tx_wake_queue(queue);
+ }
+ spin_unlock(&tfile->tx_ring.producer_lock);
+
+ if (ret) {
+ /* This should be a rare case if a qdisc is present, but
+ * can happen due to lltx.
+ * Since skb_tx_timestamp(), skb_orphan(),
+ * run_ebpf_filter() and pskb_trim() could have tinkered
+ * with the SKB, returning NETDEV_TX_BUSY is unsafe and
+ * we must drop instead.
+ */
drop_reason = SKB_DROP_REASON_FULL_RING;
goto drop;
}
/* dev->lltx requires to do our own update of trans_start */
- queue = netdev_get_tx_queue(dev, txq);
txq_trans_cond_update(queue);
/* Notify and wake up reader process */
--
2.43.0
^ permalink raw reply related
* [PATCH net-next v10 1/4] tun/tap: add ptr_ring consume helper with netdev queue wakeup
From: Simon Schippers @ 2026-05-06 14:10 UTC (permalink / raw)
To: willemdebruijn.kernel, jasowang, andrew+netdev, davem, edumazet,
kuba, pabeni, mst, eperezma, leiyang, stephen, jon, tim.gebauer,
simon.schippers, netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20260506141033.180450-1-simon.schippers@tu-dortmund.de>
Introduce tun_ring_consume() that wraps ptr_ring_consume() and calls
__tun_wake_queue(). The latter wakes the stopped netdev subqueue once
half of the ring capacity has been consumed, tracked via the new
cons_cnt field in tun_file. cons_cnt is updated while holding the ring
consumer lock, avoiding races. As a safety net, the queue is also woken
when the ring becomes empty. The point is to allow the queue to be
stopped when it gets full, which is required for traffic shaping -
implemented by the following "avoid ptr_ring tail-drop when a qdisc
is present". That patch also explains the pairing of the smp_mb()
of __tun_wake_queue().
Without the corresponding queue stopping, this patch alone causes no
regression for a tap setup sending to a qemu VM: 1.132 Mpps
to 1.144 Mpps.
Details: AMD Ryzen 5 5600X at 4.3 GHz, 3200 MHz RAM, isolated QEMU
threads, pktgen sender; Avg over 50 runs @ 100,000,000 packets;
SRSO and spectre v2 mitigations disabled.
Co-developed-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
Signed-off-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
---
drivers/net/tun.c | 54 +++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 50 insertions(+), 4 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index b183189f1853..00ecf128fe8e 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -145,6 +145,7 @@ struct tun_file {
struct list_head next;
struct tun_struct *detached;
struct ptr_ring tx_ring;
+ int cons_cnt;
struct xdp_rxq_info xdp_rxq;
};
@@ -557,6 +558,13 @@ void tun_ptr_free(void *ptr)
}
EXPORT_SYMBOL_GPL(tun_ptr_free);
+static void tun_reset_cons_cnt(struct tun_file *tfile)
+{
+ spin_lock(&tfile->tx_ring.consumer_lock);
+ tfile->cons_cnt = 0;
+ spin_unlock(&tfile->tx_ring.consumer_lock);
+}
+
static void tun_queue_purge(struct tun_file *tfile)
{
void *ptr;
@@ -564,6 +572,7 @@ static void tun_queue_purge(struct tun_file *tfile)
while ((ptr = ptr_ring_consume(&tfile->tx_ring)) != NULL)
tun_ptr_free(ptr);
+ tun_reset_cons_cnt(tfile);
skb_queue_purge(&tfile->sk.sk_write_queue);
skb_queue_purge(&tfile->sk.sk_error_queue);
}
@@ -730,6 +739,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
goto out;
}
+ tun_reset_cons_cnt(tfile);
tfile->queue_index = tun->numqueues;
tfile->socket.sk->sk_shutdown &= ~RCV_SHUTDOWN;
@@ -2115,13 +2125,46 @@ static ssize_t tun_put_user(struct tun_struct *tun,
return total;
}
-static void *tun_ring_recv(struct tun_file *tfile, int noblock, int *err)
+/* Callers must hold ring.consumer_lock */
+static void __tun_wake_queue(struct tun_struct *tun,
+ struct tun_file *tfile, int consumed)
+{
+ struct netdev_queue *txq = netdev_get_tx_queue(tun->dev,
+ tfile->queue_index);
+
+ /* Paired with smp_mb__after_atomic() in tun_net_xmit() */
+ smp_mb();
+ if (netif_tx_queue_stopped(txq)) {
+ tfile->cons_cnt += consumed;
+ if (tfile->cons_cnt >= tfile->tx_ring.size / 2 ||
+ __ptr_ring_empty(&tfile->tx_ring)) {
+ netif_tx_wake_queue(txq);
+ tfile->cons_cnt = 0;
+ }
+ }
+}
+
+static void *tun_ring_consume(struct tun_struct *tun, struct tun_file *tfile)
+{
+ void *ptr;
+
+ spin_lock(&tfile->tx_ring.consumer_lock);
+ ptr = __ptr_ring_consume(&tfile->tx_ring);
+ if (ptr)
+ __tun_wake_queue(tun, tfile, 1);
+
+ spin_unlock(&tfile->tx_ring.consumer_lock);
+ return ptr;
+}
+
+static void *tun_ring_recv(struct tun_struct *tun, struct tun_file *tfile,
+ int noblock, int *err)
{
DECLARE_WAITQUEUE(wait, current);
void *ptr = NULL;
int error = 0;
- ptr = ptr_ring_consume(&tfile->tx_ring);
+ ptr = tun_ring_consume(tun, tfile);
if (ptr)
goto out;
if (noblock) {
@@ -2133,7 +2176,7 @@ static void *tun_ring_recv(struct tun_file *tfile, int noblock, int *err)
while (1) {
set_current_state(TASK_INTERRUPTIBLE);
- ptr = ptr_ring_consume(&tfile->tx_ring);
+ ptr = tun_ring_consume(tun, tfile);
if (ptr)
break;
if (signal_pending(current)) {
@@ -2170,7 +2213,7 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
if (!ptr) {
/* Read frames from ring */
- ptr = tun_ring_recv(tfile, noblock, &err);
+ ptr = tun_ring_recv(tun, tfile, noblock, &err);
if (!ptr)
return err;
}
@@ -3406,6 +3449,8 @@ static int tun_chr_open(struct inode *inode, struct file * file)
return -ENOMEM;
}
+ tun_reset_cons_cnt(tfile);
+
mutex_init(&tfile->napi_mutex);
RCU_INIT_POINTER(tfile->tun, NULL);
tfile->flags = 0;
@@ -3614,6 +3659,7 @@ static int tun_queue_resize(struct tun_struct *tun)
for (i = 0; i < tun->numqueues; i++) {
tfile = rtnl_dereference(tun->tfiles[i]);
rings[i] = &tfile->tx_ring;
+ tun_reset_cons_cnt(tfile);
}
list_for_each_entry(tfile, &tun->disabled, next)
rings[i++] = &tfile->tx_ring;
--
2.43.0
^ permalink raw reply related
* [PATCH net-next v10 0/4] tun/tap & vhost-net: apply qdisc backpressure on full ptr_ring to reduce TX drops
From: Simon Schippers @ 2026-05-06 14:10 UTC (permalink / raw)
To: willemdebruijn.kernel, jasowang, andrew+netdev, davem, edumazet,
kuba, pabeni, mst, eperezma, leiyang, stephen, jon, tim.gebauer,
simon.schippers, netdev, linux-kernel, kvm, virtualization
This patch series deals with tun/tap & vhost-net which drop incoming
SKBs whenever their internal ptr_ring buffer is full. Instead, with this
patch series, the associated netdev queue is stopped - but only when a
qdisc is attached. If no qdisc is present the existing behavior is
preserved. The XDP transmit path is not affected. This patch series
touches tun/tap and vhost-net, as they share common logic and must be
updated together. Modifying only one of them would break the other.
By applying proper backpressure, this change allows the connected qdisc to
operate correctly, as reported in [1], and significantly improves
performance in real-world scenarios, as demonstrated in our paper [2]. For
example, we observed a 36% TCP throughput improvement for an OpenVPN
connection between Germany and the USA.
Synthetic pktgen benchmarks indicate a slight regression, but packet loss
no longer occurs. Pktgen benchmarks are provided per commit, with the final
commit showing the overall performance.
Thanks!
[1] Link: https://unix.stackexchange.com/questions/762935/traffic-shaping-ineffective-on-tun-device
[2] Link: https://cni.etit.tu-dortmund.de/storages/cni-etit/r/Research/Publications/2025/Gebauer_2025_VTCFall/Gebauer_VTCFall2025_AuthorsVersion.pdf
---
Changelog:
v10:
- Changed the term "Transmitted" to "Received" in the benchmarks,
as correctly pointed out by MST, and reran the benchmarks.
Addressed the Sashiko AI review:
- Avoid a data race on tfile->cons_cnt by always locking.
- Correctly count the number of consumed packets for vhost-net.
- Corrected a typo in the commit message of commit 3.
- Added a missing barrier on the consumer side.
--> The barriers now follow the "store buffering" principle.
- No longer return NETDEV_TX_BUSY at all, because it is unsafe.
--> Result: There are still a few drops with multiple senders, which
would be avoided by disabling LLTX.
V9: https://lore.kernel.org/netdev/20260428123859.19578-1-simon.schippers@tu-dortmund.de/
- Addressed minor nit by MST in patches 1 and 2.
- Rebased patch 3 because of commit d748047
("ptr_ring: disable KCSAN warnings").
- Documented the pair of the smp_mb__after_atomic() in tun_net_xmit()
with tun_ring_consume().
--> It simply pairs with the test_and_clear_bit() inside of
netif_wake_subqueue().
- Use 1 ptr_ring consumer spinlock instead of 2.
- Ran pktgen benchmarks with pg_set SHARED for 50 iterations on
latest kernel
--> No significant performance difference noticed
V8: https://lore.kernel.org/netdev/20260312130639.138988-1-simon.schippers@tu-dortmund.de/
- Drop code changes in drivers/net/tap.c; The code there deals with
ipvtap/macvtap which are unrelated to the goal of this patch series
and I did not realize that before
-> Greatly simplified logic, 4 instead of 9 commits
-> No more duplicated logics and distinction in vhost required
- Only wake after the queue stopped and half of the ring was consumed
as suggested by MST
-> Performance improvements for TAP, but still slightly slower
- Better benchmarking with pinned threads, XDP drop program for
tap+vhost-net and disabling CPU mitigations (and newer Ryzen 5 5600X
processor) as suggested by Jason Wang
V7: https://lore.kernel.org/netdev/20260107210448.37851-1-simon.schippers@tu-dortmund.de/
- Switch to an approach similar to veth (excluding the recently fixed
variant), as suggested by MST, with minor adjustments discussed in V6
- Rename the cover-letter title
- Add multithreaded pktgen and iperf3 benchmarks, as suggested by Jason
Wang
- Rework __ptr_ring_consume_created_space() so it can also be used after
batched consume
V6: https://lore.kernel.org/netdev/20251120152914.1127975-1-simon.schippers@tu-dortmund.de/
General:
- Major adjustments to the descriptions. Special thanks to Jon Kohler!
- Fix git bisect by moving most logic into dedicated functions and only
start using them in patch 7.
- Moved the main logic of the coupled producer and consumer into a single
patch to avoid a chicken-and-egg dependency between commits :-)
- Rebased to 6.18-rc5 and ran benchmarks again that now also include lost
packets (previously I missed a 0, so all benchmark results were higher by
factor 10...).
- Also include the benchmark in patch 7.
Producer:
- Move logic into the new helper tun_ring_produce()
- Added a smp_rmb() paired with the consumer, ensuring freed space of the
consumer is visible
- Assume that ptr_ring is not full when __ptr_ring_full_next() is called
Consumer:
- Use an unpaired smp_rmb() instead of barrier() to ensure that the
netdev_tx_queue_stopped() call completes before discarding
- Also wake the netdev queue if it was stopped before discarding and then
becomes empty
-> Fixes race with producer as identified by MST in V5
-> Waking the netdev queues upon resize is not required anymore
- Use __ptr_ring_consume_created_space() instead of messing with ptr_ring
internals
-> Batched consume now just calls
__tun_ring_consume()/__tap_ring_consume() in a loop
- Added an smp_wmb() before waking the netdev queue which is paired with
the smp_rmb() discussed above
V5: https://lore.kernel.org/netdev/20250922221553.47802-1-simon.schippers@tu-dortmund.de/T/#u
- Stop the netdev queue prior to producing the final fitting ptr_ring entry
-> Ensures the consumer has the latest netdev queue state, making it safe
to wake the queue
-> Resolves an issue in vhost-net where the netdev queue could remain
stopped despite being empty
-> For TUN/TAP, the netdev queue no longer needs to be woken in the
blocking loop
-> Introduces new helpers __ptr_ring_full_next and
__ptr_ring_will_invalidate for this purpose
- vhost-net now uses wrappers of TUN/TAP for ptr_ring consumption rather
than maintaining its own rx_ring pointer
V4: https://lore.kernel.org/netdev/20250902080957.47265-1-simon.schippers@tu-dortmund.de/T/#u
- Target net-next instead of net
- Changed to patch series instead of single patch
- Changed to new title from old title
"TUN/TAP: Improving throughput and latency by avoiding SKB drops"
- Wake netdev queue with new helpers wake_netdev_queue when there is any
spare capacity in the ptr_ring instead of waiting for it to be empty
- Use tun_file instead of tun_struct in tun_ring_recv as a more consistent
logic
- Use smp_wmb() and smp_rmb() barrier pair, which avoids any packet drops
that happened rarely before
- Use safer logic for vhost-net using RCU read locks to access TUN/TAP data
V3: https://lore.kernel.org/netdev/20250825211832.84901-1-simon.schippers@tu-dortmund.de/T/#u
- Added support for TAP and TAP+vhost-net.
V2: https://lore.kernel.org/netdev/20250811220430.14063-1-simon.schippers@tu-dortmund.de/T/#u
- Removed NETDEV_TX_BUSY return case in tun_net_xmit and removed
unnecessary netif_tx_wake_queue in tun_ring_recv.
V1: https://lore.kernel.org/netdev/20250808153721.261334-1-simon.schippers@tu-dortmund.de/T/#u
---
Simon Schippers (4):
tun/tap: add ptr_ring consume helper with netdev queue wakeup
vhost-net: wake queue of tun/tap after ptr_ring consume
ptr_ring: move free-space check into separate helper
tun/tap & vhost-net: avoid ptr_ring tail-drop when a qdisc is present
drivers/net/tun.c | 102 ++++++++++++++++++++++++++++++++++++---
drivers/vhost/net.c | 21 +++++---
include/linux/if_tun.h | 3 ++
include/linux/ptr_ring.h | 17 ++++++-
4 files changed, 129 insertions(+), 14 deletions(-)
--
2.43.0
^ permalink raw reply
* [PATCH net-next v10 2/4] vhost-net: wake queue of tun/tap after ptr_ring consume
From: Simon Schippers @ 2026-05-06 14:10 UTC (permalink / raw)
To: willemdebruijn.kernel, jasowang, andrew+netdev, davem, edumazet,
kuba, pabeni, mst, eperezma, leiyang, stephen, jon, tim.gebauer,
simon.schippers, netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20260506141033.180450-1-simon.schippers@tu-dortmund.de>
Add tun_wake_queue() to tun.c and export it for use by vhost-net. The
function validates that the file belongs to a tun/tap device,
dereferences the tun_struct under RCU, and delegates to
__tun_wake_queue().
vhost_net_buf_produce() now calls tun_wake_queue() after a successful
batched consume of the ring to allow the netdev subqueue to be woken up.
The point is to allow the queue to be stopped when it gets full, which
is required for traffic shaping - implemented by the following
"avoid ptr_ring tail-drop when a qdisc is present".
Without the corresponding queue stopping, this patch alone causes no
throughput regression for a tap+vhost-net setup sending to a qemu VM:
3.857 Mpps to 3.891 Mpps.
Details: AMD Ryzen 5 5600X at 4.3 GHz, 3200 MHz RAM, isolated QEMU
threads, XDP drop program active in VM, pktgen sender; Avg over
50 runs @ 100,000,000 packets. SRSO and spectre v2 mitigations disabled.
Co-developed-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
Signed-off-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
---
drivers/net/tun.c | 23 +++++++++++++++++++++++
drivers/vhost/net.c | 21 +++++++++++++++------
include/linux/if_tun.h | 3 +++
3 files changed, 41 insertions(+), 6 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 00ecf128fe8e..fc358c4c355b 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3776,6 +3776,29 @@ struct ptr_ring *tun_get_tx_ring(struct file *file)
}
EXPORT_SYMBOL_GPL(tun_get_tx_ring);
+/* Callers must hold ring.consumer_lock */
+void tun_wake_queue(struct file *file, int consumed)
+{
+ struct tun_file *tfile;
+ struct tun_struct *tun;
+
+ if (file->f_op != &tun_fops)
+ return;
+
+ tfile = file->private_data;
+ if (!tfile)
+ return;
+
+ rcu_read_lock();
+
+ tun = rcu_dereference(tfile->tun);
+ if (tun)
+ __tun_wake_queue(tun, tfile, consumed);
+
+ rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(tun_wake_queue);
+
module_init(tun_init);
module_exit(tun_cleanup);
MODULE_DESCRIPTION(DRV_DESCRIPTION);
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 80965181920c..ee583d6cc0fa 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -176,13 +176,21 @@ static void *vhost_net_buf_consume(struct vhost_net_buf *rxq)
return ret;
}
-static int vhost_net_buf_produce(struct vhost_net_virtqueue *nvq)
+static int vhost_net_buf_produce(struct sock *sk,
+ struct vhost_net_virtqueue *nvq)
{
+ struct file *file = sk->sk_socket->file;
struct vhost_net_buf *rxq = &nvq->rxq;
rxq->head = 0;
- rxq->tail = ptr_ring_consume_batched(nvq->rx_ring, rxq->queue,
- VHOST_NET_BATCH);
+ spin_lock(&nvq->rx_ring->consumer_lock);
+ rxq->tail = __ptr_ring_consume_batched(nvq->rx_ring, rxq->queue,
+ VHOST_NET_BATCH);
+
+ if (rxq->tail)
+ tun_wake_queue(file, rxq->tail);
+
+ spin_unlock(&nvq->rx_ring->consumer_lock);
return rxq->tail;
}
@@ -209,14 +217,15 @@ static int vhost_net_buf_peek_len(void *ptr)
return __skb_array_len_with_tag(ptr);
}
-static int vhost_net_buf_peek(struct vhost_net_virtqueue *nvq)
+static int vhost_net_buf_peek(struct sock *sk,
+ struct vhost_net_virtqueue *nvq)
{
struct vhost_net_buf *rxq = &nvq->rxq;
if (!vhost_net_buf_is_empty(rxq))
goto out;
- if (!vhost_net_buf_produce(nvq))
+ if (!vhost_net_buf_produce(sk, nvq))
return 0;
out:
@@ -995,7 +1004,7 @@ static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
unsigned long flags;
if (rvq->rx_ring)
- return vhost_net_buf_peek(rvq);
+ return vhost_net_buf_peek(sk, rvq);
spin_lock_irqsave(&sk->sk_receive_queue.lock, flags);
head = skb_peek(&sk->sk_receive_queue);
diff --git a/include/linux/if_tun.h b/include/linux/if_tun.h
index 80166eb62f41..5f3e206c7a73 100644
--- a/include/linux/if_tun.h
+++ b/include/linux/if_tun.h
@@ -22,6 +22,7 @@ struct tun_msg_ctl {
#if defined(CONFIG_TUN) || defined(CONFIG_TUN_MODULE)
struct socket *tun_get_socket(struct file *);
struct ptr_ring *tun_get_tx_ring(struct file *file);
+void tun_wake_queue(struct file *file, int consumed);
static inline bool tun_is_xdp_frame(void *ptr)
{
@@ -55,6 +56,8 @@ static inline struct ptr_ring *tun_get_tx_ring(struct file *f)
return ERR_PTR(-EINVAL);
}
+static inline void tun_wake_queue(struct file *f, int consumed) {}
+
static inline bool tun_is_xdp_frame(void *ptr)
{
return false;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH net] vsock/virtio: fix potential unbounded skb queue
From: Stefano Garzarella @ 2026-05-06 14:00 UTC (permalink / raw)
To: Arseniy Krasnov
Cc: Bobby Eshleman, Eric Dumazet, Bobby Eshleman, Stefan Hajnoczi,
Michael S. Tsirkin, David S . Miller, Jakub Kicinski, Paolo Abeni,
Simon Horman, netdev, eric.dumazet, Arseniy Krasnov, Jason Wang,
Xuan Zhuo, Eugenio Pérez, kvm, virtualization
In-Reply-To: <e1f32df5-b6a1-47a8-a783-fcc8e3c91f25@salutedevices.com>
On Wed, May 06, 2026 at 12:50:04PM +0300, Arseniy Krasnov wrote:
>
>
>05.05.2026 19:37, Bobby Eshleman wrote:
>> On Tue, May 05, 2026 at 06:11:13PM +0200, Stefano Garzarella wrote:
>>> On Tue, May 05, 2026 at 07:14:36AM -0700, Eric Dumazet wrote:
>>>> On Tue, May 5, 2026 at 6:52 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>>>>
>>>>> On Thu, Apr 30, 2026 at 12:26:52PM +0000, Eric Dumazet wrote:
>>>>>> virtio_transport_inc_rx_pkt() checks vvs->rx_bytes + len > vvs->buf_alloc.
>>>>>>
>>>>>> virtio_transport_recv_enqueue() skips coalescing for packets
>>>>>> with VIRTIO_VSOCK_SEQ_EOM.
>>>>>>
>>>>>> If fed with packets with len == 0 and VIRTIO_VSOCK_SEQ_EOM,
>>>>>> a very large number of packets can be queued
>>>>>> because vvs->rx_bytes stays at 0.
>>>>>>
>>>>>> Fix this by estimating the skb metadata size:
>>>>>>
>>>>>> (Number of skbs in the queue) * SKB_TRUESIZE(0)
>>>>>>
>>>>>> Fixes: 077706165717 ("virtio/vsock: don't use skbuff state to account credit")
>>>>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>>>>>> Cc: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>> Cc: Stefano Garzarella <sgarzare@redhat.com>
>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>>>> Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>>> Cc: "Eugenio Pérez" <eperezma@redhat.com>
>>>>>> Cc: kvm@vger.kernel.org
>>>>>> Cc: virtualization@lists.linux.dev
>>>>>> ---
>>>>>> net/vmw_vsock/virtio_transport_common.c | 4 +++-
>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>>>> index 416d533f493d7b07e9c77c43f741d28cfcd0953e..9b8014516f4fb1130ae184635fbba4dfee58bd64 100644
>>>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>>>> @@ -447,7 +447,9 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>>>>>> static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
>>>>>> u32 len)
>>>>>> {
>>>>>> - if (vvs->buf_used + len > vvs->buf_alloc)
>>>>>> + u64 skb_overhead = (skb_queue_len(&vvs->rx_queue) + 1) * SKB_TRUESIZE(0);
>>>>>> +
>>>>>> + if (skb_overhead + vvs->buf_used + len > vvs->buf_alloc)
>>>>>> return false;
>>>>>
>>>>> I'm not sure about this fix, I mean that maybe this is incomplete.
>>>>> In virtio-vsock, there is a credit mechanism between the two peers:
>>>>> https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-4850003
>>>>>
>>>>> This takes only the payload into account, so it’s true that this problem
>>>>> exists; however, perhaps we should also inform the other peer of a lower
>>>>> credit balance, otherwise the other peer will believe it has much more
>>>>> credit than it actually does, send a large payload, and then the packet
>>>>> will be discarded and the data lost (there are no retransmissions,
>>>>> etc.).
>>>>
>>>> I dunno, perhaps revert 077706165717 ("virtio/vsock: don't use skbuff
>>>> state to account credit")
>>>> and find a better fix then?
>>>
>>> IIRC the same issue was there before the commit fixed by that one (commit
>>> 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")), so
>>> not sure about reverting it TBH.
>>>
>>> CCing Arseniy and Bobby.
>
>Thanks!
>
>>>
>>>>
>>>> There is always a discrepancy between skb->len and skb->truesize.
>>>> You will not be able to announce a 1MB window, and accept one milliion
>>>> skb of 1-byte each.
>>>>
>>>> This kind of contract is broken.
>>>>
>>>
>>> Yep, I agree, but before we start discarding data (and losing it), IMHO we
>>> should at least inform the other peer that we're out of space.
>>>
>>> @Stefan, @Michael, do you think we can do something in the spec to avoid
>>> this issue and in some way take into account also the metadata in the
>>> credit. I mean to avoid the 1-byte packets flooding.
>>>
>>> Thanks,
>>> Stefano
>>>
>>>
>>
>> Indeed the old pre-fix skb code would have the same issue.
>>
>> I can't think of any way around this without extending the spec.
>
>Hi, thanks, agree with Bobby, that accounting metadata (e.g. skb size here) was not implemented "by
>design" in credit logic - another side of data exchange knows nothing about that. Also the same
>situation was before skb implementation was added by Bobby. So looks like need to update spec may be.
>
Even if we change the specifications, we still need to work with older
devices, so we should find a solution for this as well.
My main concern is data loss, so I'm considering the following options:
1. Notify the other peer of a smaller buf_alloc from the start, leave
some room for overhead, and when it's running out, notify them that
buf_alloc = 0. This way, the peer realizes it can’t send anything else.
2. Or update buf_alloc each time by removing the overhead, similar to
what’s currently done in virtio_transport_inc_rx_pkt(), but also do it
in virtio_transport_inc_tx_pkt().
As I said, IMO this patch alone is incomplete; we need to communicate
with the peer somehow regarding space. I don’t think including the
overhead in fwd_cnt is spec compliant, since the other peer has no idea
how much overhead is needed, but reducing buf_alloc should be okay, even
though I’m concerned about packets in flight.
As a quick fix, I think option 2 might be the easiest; I’ll run some
tests and send over a patch.
But in the long run, I think we absolutely need to improve memory
management in vsock, perhaps by avoiding custom solutions.
Thanks,
Stefano
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox