On 2020-08-25 13:29, Maciej Fijalkowski wrote:
On Tue, Aug 25, 2020 at 01:25:16PM +0200, Björn Töpel wrote:
On 2020-08-25 13:13, Maciej Fijalkowski wrote:
On Tue, Aug 25, 2020 at 11:16:27AM +0200, Björn Töpel wrote:
[...]
struct i40e_rx_buffer *rx_buffer;
rx_buffer = i40e_rx_bi(rx_ring, rx_ring->next_to_clean);
+ *rx_buffer_pgcnt = i40e_rx_buffer_page_count(rx_buffer);
What i previously meant was:
#if (PAGE_SIZE < 8192)
*rx_buffer_pgcnt = page_count(rx_buffer->page);
#endif
and see below
Right...
prefetchw(rx_buffer->page);
/* we are reusing so sync this buffer for CPU use */
@@ -2112,9 +2124,10 @@ static struct sk_buff *i40e_build_skb(struct i40e_ring
*rx_ring,
* either recycle the buffer or unmap it and free the associated resources.
*/
static void i40e_put_rx_buffer(struct i40e_ring *rx_ring,
- struct i40e_rx_buffer *rx_buffer)
+ struct i40e_rx_buffer *rx_buffer,
+ int rx_buffer_pgcnt)
{
- if (i40e_can_reuse_rx_page(rx_buffer)) {
+ if (i40e_can_reuse_rx_page(rx_buffer, rx_buffer_pgcnt)) {
/* hand second half of page back to the ring */
i40e_reuse_rx_page(rx_ring, rx_buffer);
} else {
@@ -2319,6 +2332,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring,
int budget)
unsigned int xdp_xmit = 0;
bool failure = false;
struct xdp_buff xdp;
+ int rx_buffer_pgcnt;
you could move scope this variable only for the
while (likely(total_rx_packets < (unsigned int)budget))
loop and init this to 0. then you could drop the helper function you've
added. and BTW the page_count is not being used for big pages but i agree
that it's better to have it set to 0.
...but isn't it a bit nasty with an output parameter that relies on the that
the input was set to zero. I guess it's a matter of taste, but I find that
more error prone.
Let me know if you have strong feelings about this, and I'll respin (but I
rather not!).
Up to you. No strong feelings, i just think that i40e_rx_buffer_page_count
is not needed. But if you want to keep it, then i was usually asking
people to provide the doxygen descriptions for newly introduced
functions... :P
but scoping it still makes sense to me, static analysis tools would agree
with me I guess.
Fair enough! I'll spin a v2! Thanks for taking a look!
Björn
Björn
#if (PAGE_SIZE < 8192)
xdp.frame_sz = i40e_rx_frame_truesize(rx_ring, 0);
@@ -2370,7 +2384,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring,
int budget)
break;
i40e_trace(clean_rx_irq, rx_ring, rx_desc, skb);
- rx_buffer = i40e_get_rx_buffer(rx_ring, size);
+ rx_buffer = i40e_get_rx_buffer(rx_ring, size, &rx_buffer_pgcnt);
/* retrieve a buffer from the ring */
if (!skb) {
@@ -2413,7 +2427,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring,
int budget)
break;
}
- i40e_put_rx_buffer(rx_ring, rx_buffer);
+ i40e_put_rx_buffer(rx_ring, rx_buffer, rx_buffer_pgcnt);
cleaned_count++;
if (i40e_is_non_eop(rx_ring, rx_desc, skb))
--
2.25.1
