Re: [Linuxptp-devel] [PATCH v5 1/2] pmc_agent: Add option to run callback for signaling messages

2023-04-26 Thread Erez 
On Wed, 26 Apr 2023 at 11:55, Maciek Machnikowski 
wrote:

> On 4/21/2023 6:46 PM, Erez wrote:
> >
> >
> > On Fri, 21 Apr 2023 at 17:27, Maciek Machnikowski
> > mailto:mac...@machnikowski.net>> wrote:
> >
> > On 4/21/2023 1:25 PM, Erez wrote:
> > >
> > >
> > > On Thu, 20 Apr 2023 at 19:01, Maciek Machnikowski
> > > mailto:mac...@machnikowski.net>
> > >>
> > wrote:
> > >
> > > Add option to run callback that when the PMC agent receives
> > > signaling messages.
> > >
> > > Signed-off-by: Maciek Machnikowski  > 
> > > >>
> > > ---
> > >  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   jne49f 
> >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 
> msg_type(*msg) == SIGNALING) {
>  487:   48 8b 45 c8 mov-0x38(%rbp),%rax
>  48b:   48 8b 00mov(%rax),%rax
>  48e:   48 89 c7mov%rax,%rdi
>  491:   e8 39 fc ff ff  callq  cf 
>
> Do you know any architecture that's different than this?
>

There are about 12 different architectures, not to mention 32 and 64 bit
and old 8 and 16 bits.
But this group is not about machine code and assembler.
I suggest we leave it out. This argument does not fly anywhere.


>
> >
> >
> > /* Skip callback if message is not management */
> > skip_cb = (res <= 0) ? true : false;
> >  54c:   b94047e0ldr w0, [sp, #68]
> >  550:   711fcmp w0, #0x0
> >  554:   1a9fc7e0csetw0,

Re: [Linuxptp-devel] [PATCH v5 1/2] pmc_agent: Add option to run callback for signaling messages

2023-04-26 Thread Maciek Machnikowski 
On 4/21/2023 6:46 PM, Erez wrote:
> 
> 
> On Fri, 21 Apr 2023 at 17:27, Maciek Machnikowski
> mailto:mac...@machnikowski.net>> wrote:
> 
> On 4/21/2023 1:25 PM, Erez wrote:
> >
> >
> > On Thu, 20 Apr 2023 at 19:01, Maciek Machnikowski
> > mailto:mac...@machnikowski.net>
> >>
> wrote:
> >
> >     Add option to run callback that when the PMC agent receives
> >     signaling messages.
> >
> >     Signed-off-by: Maciek Machnikowski  
> >     >>
> >     ---
> >      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   jne49f 
>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 
msg_type(*msg) == SIGNALING) {
 487:   48 8b 45 c8 mov-0x38(%rbp),%rax
 48b:   48 8b 00mov(%rax),%rax
 48e:   48 89 c7mov%rax,%rdi
 491:   e8 39 fc ff ff  callq  cf 

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:   711f        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:   711f        cmp     w0, #0x0
>  568:   54e0        b.eq    584   // b.none
>  56c:   f9400fe0        ldr     x0, [sp, #24]
>  570:   f940 

[Linuxptp-devel] [PATCH v5 1/2] pmc_agent: Add option to run callback for signaling messages

2023-04-20 Thread Maciek Machnikowski 
Add option to run callback that when the PMC agent receives signaling messages.

Signed-off-by: Maciek Machnikowski 
---
 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)) {
+   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)) {
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
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel