Message-Id: <20250903145600.512627-1-jackzxcui1989@163.com>
Date: Wed,  3 Sep 2025 22:56:00 +0800
From: Xin Zhao <jackzxcui1989@....com>
To: kerneljasonxing@...il.com,
	willemdebruijn.kernel@...il.com,
	edumazet@...gle.com,
	ferenc@...es.dev
Cc: davem@...emloft.net,
	kuba@...nel.org,
	pabeni@...hat.com,
	horms@...nel.org,
	netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v10 1/2] net: af_packet: remove last_kactive_blk_num field

On Tue, Sep 2, 2025 at 22:04 PM Jason Xing <kerneljasonxing@...il.com> 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.

This diff file may not clearly demonstrate the changes made to the
prb_retire_rx_blk_timer_expired function. We have simply removed the check for
pkc->last_kactive_blk_num == pkc->kactive_blk_num; the other logic remains unchanged.
Therefore, we should only need to explain why removing the check for
pkc->last_kactive_blk_num == pkc->kactive_blk_num will not cause any negative impacts.

I will clarify in the commit that our change to prb_retire_rx_blk_timer_expired only
involves removing the check for pkc->last_kactive_blk_num == pkc->kactive_blk_num,
to ensure that everyone understands this point, as it may not be clearly visible from
the diff file.

The commit may be changed as follow:

kactive_blk_num (K) is only incremented on block close.
In timer callback prb_retire_rx_blk_timer_expired, except delete_blk_timer
is true, last_kactive_blk_num (L) is set to match kactive_blk_num (K) in
all cases. L is also set to match K in prb_open_block.
The only case K not equal to L is when scheduled by tpacket_rcv
and K is just incremented on block close but no new block could be opened,
so that it does not call prb_open_block in prb_dispatch_next_block.
This patch modifies the prb_retire_rx_blk_timer_expired function by simply 
removing the check for L == K. Why can we remove the check for L == K in
timer callback?
In prb_freeze_queue, reset_pending_on_curr_blk (R) is set to 1. If R == 1,
prb_queue_frozen return 1. In prb_retire_rx_blk_timer_expired,
frozen = prb_queue_frozen(pkc); so frozen is 1 when R == 1.

Consider the following case:
(before applying this patch)
cpu0                                  cpu1
tpacket_rcv
  ...
    prb_dispatch_next_block
      prb_freeze_queue (R = 1)
                                      prb_retire_rx_blk_timer_expired
                                        L != K
                                          _prb_refresh_rx_retire_blk_timer
                                            refresh timer
                                            set L = K
(after applying this patch)
cpu0                                  cpu1
tpacket_rcv
  ...
    prb_dispatch_next_block
      prb_freeze_queue (R = 1)
                                      prb_retire_rx_blk_timer_expired
                                        !forzen is 0
                                          check prb_curr_blk_in_use
                                            if true
                                              same as (before apply)
                                            if false
                                              prb_open_block
Before applying this patch, prb_retire_rx_blk_timer_expired will do nothing
but refresh timer and set L = K in the case above. After applying this
patch, it will check prb_curr_blk_in_use and call prb_open_block if
user-space caught up.


Please check if the above description is appropriate. I will change the
description as above in PATCH v11.


> 
> >
> > Signed-off-by: Xin Zhao <jackzxcui1989@....com>
> 
> It was suggested by Willem, so please add:
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@...il.com>

Okay, I will add it in the PATCH v11.

> 
> So far, it looks good to me as well:
> Reviewed-by: Jason Xing <kerneljasonxing@...il.com>
> 
> And I will finish reviewing the other patch by tomorrow :)


Thanks
Xin Zhao

Powered by blists - more mailing lists