Re: [PATCH net-next v10 1/2] net: af_packet: remove last_kactive_blk_num field
From: Jason Xing <hidden>
Date: 2025-09-02 14:05:32
Also in:
lkml
On Sun, Aug 31, 2025 at 6:10 PM Xin Zhao [off-list ref] wrote:
kactive_blk_num (K) is incremented on block close. last_kactive_blk_num (L) is set to match K on block open and each timer. So the only time that they differ is if a block is closed in tpacket_rcv and no new block could be opened. So the origin check L==K in timer callback only skip the case 'no new block to open'. If we remove L==K check, it will make prb_curr_blk_in_use check earlier, which will not cause any side effect.
I believe the above commit message needs to be revised: 1) the above sentence (starting from 'if we remove L....') means nothing because your modification doesn't change the behaviour when the queue is not frozen. 2) lack of proofs/reasons on why exposing the prb_open_block() logic doesn't cause side effects. It's the key proof that shows to future readers to make sure this patch will not bring trouble.
Signed-off-by: Xin Zhao <redacted>
It was suggested by Willem, so please add: Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com> So far, it looks good to me as well: Reviewed-by: Jason Xing <redacted> And I will finish reviewing the other patch by tomorrow :) Thanks, Jason
quoted hunk ↗ jump to hunk
--- net/packet/af_packet.c | 60 ++++++++++++++++++++---------------------- net/packet/internal.h | 6 ----- 2 files changed, 28 insertions(+), 38 deletions(-)diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index a7017d7f0..d4eb4a4fe 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c@@ -669,7 +669,6 @@ static void init_prb_bdqc(struct packet_sock *po, p1->knum_blocks = req_u->req3.tp_block_nr; p1->hdrlen = po->tp_hdrlen; p1->version = po->tp_version; - p1->last_kactive_blk_num = 0; po->stats.stats3.tp_freeze_q_cnt = 0; if (req_u->req3.tp_retire_blk_tov) p1->retire_blk_tov = req_u->req3.tp_retire_blk_tov;@@ -693,7 +692,6 @@ static void _prb_refresh_rx_retire_blk_timer(struct tpacket_kbdq_core *pkc) { mod_timer(&pkc->retire_blk_timer, jiffies + pkc->tov_in_jiffies); - pkc->last_kactive_blk_num = pkc->kactive_blk_num; } /*@@ -750,38 +748,36 @@ static void prb_retire_rx_blk_timer_expired(struct timer_list *t) write_unlock(&pkc->blk_fill_in_prog_lock); } - if (pkc->last_kactive_blk_num == pkc->kactive_blk_num) { - if (!frozen) { - if (!BLOCK_NUM_PKTS(pbd)) { - /* An empty block. Just refresh the timer. */ - goto refresh_timer; - } - prb_retire_current_block(pkc, po, TP_STATUS_BLK_TMO); - if (!prb_dispatch_next_block(pkc, po)) - goto refresh_timer; - else - goto out; + if (!frozen) { + if (!BLOCK_NUM_PKTS(pbd)) { + /* An empty block. Just refresh the timer. */ + goto refresh_timer; + } + prb_retire_current_block(pkc, po, TP_STATUS_BLK_TMO); + if (!prb_dispatch_next_block(pkc, po)) + goto refresh_timer; + else + goto out; + } else { + /* Case 1. Queue was frozen because user-space was + * lagging behind. + */ + if (prb_curr_blk_in_use(pbd)) { + /* + * Ok, user-space is still behind. + * So just refresh the timer. + */ + goto refresh_timer; } else { - /* Case 1. Queue was frozen because user-space was - * lagging behind. + /* Case 2. queue was frozen,user-space caught up, + * now the link went idle && the timer fired. + * We don't have a block to close.So we open this + * block and restart the timer. + * opening a block thaws the queue,restarts timer + * Thawing/timer-refresh is a side effect. */ - if (prb_curr_blk_in_use(pbd)) { - /* - * Ok, user-space is still behind. - * So just refresh the timer. - */ - goto refresh_timer; - } else { - /* Case 2. queue was frozen,user-space caught up, - * now the link went idle && the timer fired. - * We don't have a block to close.So we open this - * block and restart the timer. - * opening a block thaws the queue,restarts timer - * Thawing/timer-refresh is a side effect. - */ - prb_open_block(pkc, pbd); - goto out; - } + prb_open_block(pkc, pbd); + goto out; } }diff --git a/net/packet/internal.h b/net/packet/internal.h index 1e743d031..d367b9f93 100644 --- a/net/packet/internal.h +++ b/net/packet/internal.h@@ -24,12 +24,6 @@ struct tpacket_kbdq_core { unsigned short kactive_blk_num; unsigned short blk_sizeof_priv; - /* last_kactive_blk_num: - * trick to see if user-space has caught up - * in order to avoid refreshing timer when every single pkt arrives. - */ - unsigned short last_kactive_blk_num; - char *pkblk_start; char *pkblk_end; int kblk_size; --2.34.1