All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>
Cc: "Mattias Rönnblom" <hofors@lysator.liu.se>,
	"Mattias Rönnblom" <mattias.ronnblom@ericsson.com>,
	"John W . Linville" <linville@tuxdriver.com>,
	dev@dpdk.org, "Tyler Retzlaff" <roretzla@linux.microsoft.com>,
	"Honnappa Nagarahalli" <Honnappa.Nagarahalli@arm.com>
Subject: Re: [PATCH] net/af_packet: cache align Rx/Tx structs
Date: Wed, 24 Apr 2024 11:28:18 +0100	[thread overview]
Message-ID: <Zijewu5AFTPj0Q88@bricha3-mobl1.ger.corp.intel.com> (raw)
In-Reply-To: <5d2a0887-527a-4948-943c-65f1dfda9328@amd.com>

On Wed, Apr 24, 2024 at 11:21:52AM +0100, Ferruh Yigit wrote:
> On 4/23/2024 9:56 PM, Mattias Rönnblom wrote:
> > On 2024-04-23 13:15, Ferruh Yigit wrote:
> >> On 4/23/2024 10:08 AM, Mattias Rönnblom wrote:
> >>> Cache align Rx and Tx queue struct to avoid false sharing.
> >>>
> >>> RX struct happens to be 64 bytes on x86_64 already, so cache
> >>> alignment makes no change there, but it does on 32-bit ISAs.
> >>>
> >>> TX struct is 56 bytes on x86_64.
> >>>
> >>
> >> Hi Mattias,
> >>
> >> No objection to the patch. Is the improvement theoretical or do you
> >> measure any improvement practically, if so how much is the
> >> improvement?
> >>
> > 
> > I didn't run any benchmarks.
> > 
> > Two cores storing to a (falsely) shared cache line on a per-packet
> > basis is going to be very expensive, at least for "light touch"
> > applications.
> > 
> 
> ack I expect for af_packet bottleneck is the kernel side, so I don't
> expect any visible improvement practically, but OK to fix this
> theoretical issue.
> 
> >>> Both structs keep counters, and in the RX case they are updated even
> >>> for empty polls.
> >>>
> >>
> >> Do you think does it help if move 'rx_pkts' & 'rx_bytes' update within
> >> the loop?
> >>
> > 
> > No, why? Wouldn't that be worse? Especially since rx_pkts and rx_bytes
> > are declared volatile, so you are forcing a load-modify-store cycle for
> > every increment.
> > 
> 
> My intention was to prevent updating stats in empty polls, I thought
> stats will be hot in the cache but won't work with volatile.
> 
Yes, it will. Volatile only prevents caching in registers, it does not
affect the storing of data within the cache hierarchy. Reads/writes of
stats on empty polls should indeed hit the L1 as expected. However, that is
still less efficient than just doing a register increment which could
theoretically be the result without the volatile.

/Bruce

  reply	other threads:[~2024-04-24 10:28 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-23  9:08 [PATCH] net/af_packet: cache align Rx/Tx structs Mattias Rönnblom
2024-04-23 11:15 ` Ferruh Yigit
2024-04-23 20:56   ` Mattias Rönnblom
2024-04-24  0:27     ` Honnappa Nagarahalli
2024-04-24  6:28       ` Mattias Rönnblom
2024-04-24 10:21     ` Ferruh Yigit
2024-04-24 10:28       ` Bruce Richardson [this message]
2024-04-24 18:02         ` Ferruh Yigit
2024-04-24 11:57       ` Mattias Rönnblom
2024-04-24 17:50         ` Ferruh Yigit
2024-04-24 19:13           ` Stephen Hemminger
2024-04-24 22:27             ` Mattias Rönnblom
2024-04-24 23:55               ` Stephen Hemminger
2024-04-25  9:26                 ` Mattias Rönnblom
2024-04-25  9:49                   ` Morten Brørup
2024-04-25 14:04                   ` Ferruh Yigit
2024-04-25 15:06                     ` Mattias Rönnblom
2024-04-25 16:21                       ` Ferruh Yigit
2024-04-25 15:07                     ` Stephen Hemminger
2024-04-25 14:08   ` Ferruh Yigit
2024-04-25 15:08     ` Mattias Rönnblom
2024-04-25 15:35       ` Ferruh Yigit
2024-04-26  7:25         ` Mattias Rönnblom
2024-04-26  7:38 ` Mattias Rönnblom
2024-04-26  8:27   ` Ferruh Yigit
2024-04-26 10:20     ` Mattias Rönnblom
2024-04-26  9:05   ` [PATCH v3] " Mattias Rönnblom
2024-04-26  9:22     ` Morten Brørup
2024-04-26 15:10     ` Stephen Hemminger
2024-04-26 15:41     ` Tyler Retzlaff
2024-04-29  8:46       ` Ferruh Yigit
2024-04-26 21:27 ` [PATCH] " Patrick Robb

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Zijewu5AFTPj0Q88@bricha3-mobl1.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@amd.com \
    --cc=hofors@lysator.liu.se \
    --cc=linville@tuxdriver.com \
    --cc=mattias.ronnblom@ericsson.com \
    --cc=roretzla@linux.microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.