On 4/21/2023 6:46 PM, Erez wrote:
>
>
> On Fri, 21 Apr 2023 at 17:27, Maciek Machnikowski
> <[email protected] <mailto:[email protected]>> wrote:
>
> On 4/21/2023 1:25 PM, Erez wrote:
> >
> >
> > On Thu, 20 Apr 2023 at 19:01, Maciek Machnikowski
> > <[email protected] <mailto:[email protected]>
> <mailto:[email protected] <mailto:[email protected]>>>
> wrote:
> >
> > Add option to run callback that when the PMC agent receives
> > signaling messages.
> >
> > Signed-off-by: Maciek Machnikowski <[email protected]
> <mailto:[email protected]>
> > <mailto:[email protected] <mailto:[email protected]>>>
> > ---
> > pmc_agent.c | 21 +++++++++++++++++++--
> > pmc_agent.h | 7 +++++++
> > 2 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/pmc_agent.c b/pmc_agent.c
> > index 62d1a86..ef4e1bb 100644
> > --- a/pmc_agent.c
> > +++ b/pmc_agent.c
> > @@ -42,6 +42,7 @@ struct pmc_agent {
> > bool dds_valid;
> > int leap;
> > int pmc_ds_requested;
> > + bool signaling_cb_ena;
> > bool stay_subscribed;
> > int sync_offset;
> > int utc_offset_traceable;
> > @@ -127,6 +128,7 @@ static int run_pmc(struct pmc_agent *node, int
> > timeout, int ds_id,
> > #define N_FD 1
> > struct pollfd pollfd[N_FD];
> > int cnt, res;
> > + bool skip_cb;
> >
> > while (1) {
> > pollfd[0].fd = pmc_get_transport_fd(node->pmc);
> > @@ -178,9 +180,18 @@ static int run_pmc(struct pmc_agent
> *node, int
> > timeout, int ds_id,
> > node->pmc_ds_requested = 0;
> > return RUN_PMC_NODEV;
> > }
> > - if (res <= 0 ||
> > +
> > + /* Skip callback if message is not management */
> > + skip_cb = (res <= 0) ? true : false;
> > +
> > + /* Run the callback on signaling messages if
> > configured */
> > + if (node->signaling_cb_ena && (msg_type(*msg) ==
> > SIGNALING)) {
> >
> >
> > I would add res == 0, to save time here.
> >
> > You can have
> > if (res < 0) {
> > skip_cb = false;
> > } else if(res == 0) {
> > if (node->signaling_cb_ena && (msg_type(*msg) ==
> > SIGNALING)) {
> > skip_cb = true;
> > } else {
> > skip_cb = false;
> > }
> > } else {
> > skip_cb = true;
> > }
> >
> > This code saves you from checking 'node->signaling_cb_ena' if res
> is not 0.
> > 'res' is a local variable, 'node->signaling_cb_ena' is referred
> through> a pointer, so its fetch time is much bigger.
>
> This is not true. It compiles to a single ASM line even without any
> optimizations enabled:
>
>
> This looks like arm64. There are other CPUs beside ARM.
> And in assembler it is hard to "see" how long it takes to load
> 'signaling_cb_ena' from node unless it is in the memory cache.
It's similar on x86.
/* Run the callback on signaling messages if configured */
if (res == 0 && node->signaling_cb_ena &&
475: 83 7d f4 00 cmpl $0x0,-0xc(%rbp)
479: 75 24 jne 49f <run_pmc+0x1e0>
>47b: 48 8b 45 d8 mov -0x28(%rbp),%rax
>47f: 0f b6 40 30 movzbl 0x30(%rax),%eax
>483: 84 c0 test %al,%al
485: 74 18 je 49f <run_pmc+0x1e0>
msg_type(*msg) == SIGNALING) {
487: 48 8b 45 c8 mov -0x38(%rbp),%rax
48b: 48 8b 00 mov (%rax),%rax
48e: 48 89 c7 mov %rax,%rdi
491: e8 39 fc ff ff callq cf <msg_type>
Do you know any architecture that's different than this?
>
>
> /* Skip callback if message is not management */
> skip_cb = (res <= 0) ? true : false;
> 54c: b94047e0 ldr w0, [sp, #68]
> 550: 7100001f cmp w0, #0x0
> 554: 1a9fc7e0 cset w0, le
> 558: 39013fe0 strb w0, [sp, #79]
> /* Run the callback on signaling messages if configured */
> if (node->signaling_cb_ena && (msg_type(*msg) ==
> SIGNALING)) {
> 55c: f94017e0 ldr x0, [sp, #40]
> >560: 3940c000 ldrb w0, [x0, #48]
> 564: 7100001f cmp w0, #0x0
> 568: 540000e0 b.eq 584 <run_pmc+0x210> // b.none
> 56c: f9400fe0 ldr x0, [sp, #24]
> 570: f9400000 ldr x0, [x0]
> 574: 97fffeea bl 11c <msg_type>
> 578: 7100301f cmp w0, #0xc
> 57c: 54000041 b.ne <http://b.ne> 584 <run_pmc+0x210>
> // b.any
> skip_cb = false;
> 580: 39013fff strb wzr, [sp, #79]
> }
>
>
> Adding a check for res adds 3 more ASM instructions:
>
>
> But save loading `signaling_cb_ena` if you mainly use management messages!
>
>
>
> /* Run the callback on signaling messages if
> configured */
> if (res == 0 && node->signaling_cb_ena &&
> >55c: b94047e0 ldr w0, [sp, #68]
> >560: 7100001f cmp w0, #0x0
> >564: 54000161 b.ne <http://b.ne> 590 <run_pmc+0x21c>
> // b.any
> 568: f94017e0 ldr x0, [sp, #40]
> 56c: 3940c000 ldrb w0, [x0, #48]
> 570: 7100001f cmp w0, #0x0
> 574: 540000e0 b.eq 590 <run_pmc+0x21c> // b.none
> msg_type(*msg) == SIGNALING) {
> 578: f9400fe0 ldr x0, [sp, #24]
> 57c: f9400000 ldr x0, [x0]
> 580: 97fffee7 bl 11c <msg_type>
> if (res == 0 && node->signaling_cb_ena &&
> 584: 7100301f cmp w0, #0xc
> 588: 54000041 b.ne <http://b.ne> 590 <run_pmc+0x21c>
> // b.any
> skip_cb = false;
> 58c: 39013fff strb wzr, [sp, #79]
> }
>
> Thanks,
> Maciek
>
>
> Again you can simply reuse `res` and have a small code and also faster code.
> I do not understand why you are afraid of using reuse `res`.
>
> I only point on optimization. I assume that most cases are
> management messages, and testing the signaling flag for all packets does
> not help.
>
> If you want a better solution, you replace in run_pmc() :
>
> type = msg_type(msg);
> switch (type) {
> case SIGNALING:
> res = node->signaling_cb_ena ? 2 : 0;
> break;
> case MANAGEMENT:
> res = is_msg_mgt(msg);
> break;
> default:
> break
> }
>
> And remove the 'msg_type(msg) != MANAGEMENT' from is_msg_mgt().
Optimizing run_pmc is beyond the scope of my patch. I'll add a check for
res==0 before checking node->signaling_cb_ena, but I hardly believe
it'll have any performance benefit.
If you run the run_pmc infrequently, this check will not matter, as it
takes only a couple of ns to read from memory. You'll spend much more
time on function calls preceding this check.
If you call run_pmc constantly, then this location will most likely be
cached, as it holds a static value, and it will still not negatively
impact the performance. To see any difference you'd need to hit a ton of
messages/second - is there any use case that requires it?
Thanks
Maciek
>
> Assembler rarely helps to understand speed and size of code.
> Especially code that runs on multiple CPUs as free software.
>
>
>
>
>
> Erez
>
>
>
>
> > This code is bigger, but you do not want to reuse the 'res' variable.
> >
> >
> >
> > + skip_cb = false;
> > + }
> > +
> > + if (skip_cb ||
> > node->recv_subscribed(node->recv_context,
> *msg,
> > ds_id) ||
> > - management_tlv_id(*msg) != ds_id) {
> > + (res == 1 && management_tlv_id(*msg) !=
> ds_id)) {
> >
> >
> > This part is OK.
> >
> > Erez
> >
> >
> >
> > msg_put(*msg);
> > *msg = NULL;
> > continue;
> > @@ -430,3 +441,9 @@ bool pmc_agent_utc_offset_traceable(struct
> > pmc_agent *agent)
> > {
> > return agent->utc_offset_traceable;
> > }
> > +
> > +void pmc_agent_enable_signaling_cb(struct pmc_agent *agent, bool
> > enable)
> > +{
> > + agent->signaling_cb_ena = enable;
> > +}
> > +
> > diff --git a/pmc_agent.h b/pmc_agent.h
> > index 2fb1cc8..9ae37f7 100644
> > --- a/pmc_agent.h
> > +++ b/pmc_agent.h
> > @@ -170,4 +170,11 @@ int pmc_agent_update(struct pmc_agent
> *agent);
> > */
> > bool pmc_agent_utc_offset_traceable(struct pmc_agent *agent);
> >
> > +/**
> > + * Enables or disables callback on signaling messages
> > + * @param agent Pointer to a PMC instance obtained via @ref
> > pmc_agent_create().
> > + * @param enable - if set to true, callback will be called on
> > signaling msgs
> > + */
> > +void pmc_agent_enable_signaling_cb(struct pmc_agent *agent, bool
> > enable);
> > +
> > #endif
> > --
> > 2.30.2
> >
>
_______________________________________________
Linuxptp-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
