[v6,4/5] examples/kni: add log msgs to show and clear stats
Checks
Commit Message
Add logging messages showing the commands necessary for the user to
have the application display and zero the statistics.
Signed-off-by: Dan Gora <dg@adax.com>
---
examples/kni/main.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
Comments
On Wed, 24 Oct 2018 17:27:18 -0300
Dan Gora <dg@adax.com> wrote:
> + pid = getpid();
> + RTE_LOG(INFO, APP, "========================\n");
> + RTE_LOG(INFO, APP, "KNI Running\n");
> + RTE_LOG(INFO, APP, "kill -SIGUSR1 %d\n", pid);
> + RTE_LOG(INFO, APP, " Show KNI Statistics.\n");
> + RTE_LOG(INFO, APP, "kill -SIGUSR2 %d\n", pid);
> + RTE_LOG(INFO, APP, " Zero KNI Statistics.\n");
> + RTE_LOG(INFO, APP, "========================\n");
> + fflush(stdout);
> +
Thanks for updating the example to cover the new code.
This seems like an overly wordy message which should really be in the documentation
not a billboard in the code.
In my opinion, having verbose messages is unhelpful since it just clutters
the experience.
On Wed, Oct 24, 2018 at 5:46 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
> > + pid = getpid();
> > + RTE_LOG(INFO, APP, "========================\n");
> > + RTE_LOG(INFO, APP, "KNI Running\n");
> > + RTE_LOG(INFO, APP, "kill -SIGUSR1 %d\n", pid);
> > + RTE_LOG(INFO, APP, " Show KNI Statistics.\n");
> > + RTE_LOG(INFO, APP, "kill -SIGUSR2 %d\n", pid);
> > + RTE_LOG(INFO, APP, " Zero KNI Statistics.\n");
> > + RTE_LOG(INFO, APP, "========================\n");
> > + fflush(stdout);
> > +
>
> Thanks for updating the example to cover the new code.
>
> This seems like an overly wordy message which should really be in the documentation
> not a billboard in the code.
>
> In my opinion, having verbose messages is unhelpful since it just clutters
> the experience.
Sigh...
This is version 6 of this patch. You could have said something about
it at any point in the last two and a half months that I have been
struggling to get this merged.
These "features" were never documented at all, so you would have no
idea they existed unless you read the code.
The point of this patch is that you can just copy and paste the
commands directly from the screen. This saves you from having to type
'ps -ef|grep kni', cut the PID, type 'kill -SIGUSR1', then paste the
PID. How is that easier that what I have done?
And it's not a billboard, it's 7 lines. Have you actually tried it?
The amount of nitpicking on these patches has been just incredible..
People get entire subsystems merged with 1/10th the hassle that I've
been given to add one stupid function. It's extremely frustrating.
I've totally given up on trying to get my other KNI patches merged..
It's just not worth it..
dan
On Wed, 24 Oct 2018 17:56:50 -0300
Dan Gora <dg@adax.com> wrote:
> On Wed, Oct 24, 2018 at 5:46 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> > > + pid = getpid();
> > > + RTE_LOG(INFO, APP, "========================\n");
> > > + RTE_LOG(INFO, APP, "KNI Running\n");
> > > + RTE_LOG(INFO, APP, "kill -SIGUSR1 %d\n", pid);
> > > + RTE_LOG(INFO, APP, " Show KNI Statistics.\n");
> > > + RTE_LOG(INFO, APP, "kill -SIGUSR2 %d\n", pid);
> > > + RTE_LOG(INFO, APP, " Zero KNI Statistics.\n");
> > > + RTE_LOG(INFO, APP, "========================\n");
> > > + fflush(stdout);
> > > +
> >
> > Thanks for updating the example to cover the new code.
> >
> > This seems like an overly wordy message which should really be in the documentation
> > not a billboard in the code.
> >
> > In my opinion, having verbose messages is unhelpful since it just clutters
> > the experience.
>
> Sigh...
>
> This is version 6 of this patch. You could have said something about
> it at any point in the last two and a half months that I have been
> struggling to get this merged.
>
> These "features" were never documented at all, so you would have no
> idea they existed unless you read the code.
>
> The point of this patch is that you can just copy and paste the
> commands directly from the screen. This saves you from having to type
> 'ps -ef|grep kni', cut the PID, type 'kill -SIGUSR1', then paste the
> PID. How is that easier that what I have done?
>
> And it's not a billboard, it's 7 lines. Have you actually tried it?
>
> The amount of nitpicking on these patches has been just incredible..
> People get entire subsystems merged with 1/10th the hassle that I've
> been given to add one stupid function. It's extremely frustrating.
> I've totally given up on trying to get my other KNI patches merged..
> It's just not worth it..
>
> dan
I look at patches as they show up and don't want to overwhelm people
with a long laundry list of items. Just a case of call them as I see them.
Often a developer is focused on "does my feature work" and misses how
the new feature is not used by most people.
Remember when working on projects that the unstated policy is that all
code should look the same. Anything you introduce should look like everything
around it. Yes, this limits taste and individual freedom, but if you want to
change things then doing it in new code is not the way to do it.
The patch can go in as is. There is no reason for a message to block that.
Just trying to see what can be improved.
Don't get disheartened, 6 versions of a patch is nothing bad.
Sometimes it takes 20 or more until agreement occurs.
On Wed, Oct 24, 2018 at 6:18 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
> > >
> > > This seems like an overly wordy message which should really be in the documentation
> > > not a billboard in the code.
> > >
> > > In my opinion, having verbose messages is unhelpful since it just clutters
> > > the experience.
> >
> > Sigh...
> >
> > This is version 6 of this patch. You could have said something about
> > it at any point in the last two and a half months that I have been
> > struggling to get this merged.
> >
> > These "features" were never documented at all, so you would have no
> > idea they existed unless you read the code.
> >
> > The point of this patch is that you can just copy and paste the
> > commands directly from the screen. This saves you from having to type
> > 'ps -ef|grep kni', cut the PID, type 'kill -SIGUSR1', then paste the
> > PID. How is that easier that what I have done?
> >
> > And it's not a billboard, it's 7 lines. Have you actually tried it?
> >
> > The amount of nitpicking on these patches has been just incredible..
> > People get entire subsystems merged with 1/10th the hassle that I've
> > been given to add one stupid function. It's extremely frustrating.
> > I've totally given up on trying to get my other KNI patches merged..
> > It's just not worth it..
> >
> > dan
>
> I look at patches as they show up and don't want to overwhelm people
> with a long laundry list of items. Just a case of call them as I see them.
I personally, and I would imagine that most people, would prefer to
get a long list of items to fix so that they can all be fixed at one
time rather than this endless churning on the mailing list and the
overhead of source control, formatting the patches, sending the
patches, etc...
> Often a developer is focused on "does my feature work" and misses how
> the new feature is not used by most people.
>
> Remember when working on projects that the unstated policy is that all
> code should look the same. Anything you introduce should look like everything
> around it. Yes, this limits taste and individual freedom, but if you want to
> change things then doing it in new code is not the way to do it.
By definition, to change things, you need new code.
In my experience 99.9999% of users do not read the documentation.
Engineers, and unix/linux engineers in particular, are notorious for
being terrible UI developers. The KNI sample application has a
*terrible* user interface. You run the application and.... nothing...
no indication of what happened, no indication of what you should do,
just some nonsensical EAL debug messages.
I was just trying to give the user some sense that 1: the thing is
actually running and doing something 2: there is something that you
can do to interact with the application and 3: save myself from some
extra typing. I figured that since I hate any extra typing that most
other people would as well.
As far as it looking like everything around it; there was nothing
around it before, so there's nothing to compare it against... I
*could* have implemented a full menu thing like testpmd, but my
objective here was not to fix the KNI sample app, it was just to add
rte_kni_update_link(). That's all I really wanted...
> The patch can go in as is. There is no reason for a message to block that.
> Just trying to see what can be improved.
>
> Don't get disheartened, 6 versions of a patch is nothing bad.
> Sometimes it takes 20 or more until agreement occurs.
In the 6 months or so that I've been actively working on DPDK, I've
never seen one, especially to add one, basically trivial, function.
Two patches that caused compilation errors got merged in the last
couple of weeks.
I'll send a version 7 and add these commands to the documentation in
the other patch [3/5].. You guys can accept or reject this patch
[4/5]...
@@ -171,14 +171,13 @@ signal_handler(int signum)
/* When we receive a USR2 signal, reset stats */
if (signum == SIGUSR2) {
memset(&kni_stats, 0, sizeof(kni_stats));
- printf("\n**Statistics have been reset**\n");
+ printf("\n** Statistics have been reset **\n");
return;
}
/* When we receive a RTMIN or SIGINT signal, stop kni processing */
if (signum == SIGRTMIN || signum == SIGINT){
- printf("SIGRTMIN is received, and the KNI processing is "
- "going to stop\n");
+ printf("\nSIGRTMIN/SIGINT received. KNI processing stopping.\n");
rte_atomic32_inc(&kni_stop);
return;
}
@@ -950,6 +949,7 @@ main(int argc, char** argv)
unsigned i;
void *retval;
pthread_t kni_link_tid;
+ int pid;
/* Associate signal_hanlder function with USR signals */
signal(SIGUSR1, signal_handler);
@@ -1006,6 +1006,16 @@ main(int argc, char** argv)
}
check_all_ports_link_status(ports_mask);
+ pid = getpid();
+ RTE_LOG(INFO, APP, "========================\n");
+ RTE_LOG(INFO, APP, "KNI Running\n");
+ RTE_LOG(INFO, APP, "kill -SIGUSR1 %d\n", pid);
+ RTE_LOG(INFO, APP, " Show KNI Statistics.\n");
+ RTE_LOG(INFO, APP, "kill -SIGUSR2 %d\n", pid);
+ RTE_LOG(INFO, APP, " Zero KNI Statistics.\n");
+ RTE_LOG(INFO, APP, "========================\n");
+ fflush(stdout);
+
ret = rte_ctrl_thread_create(&kni_link_tid,
"KNI link status check", NULL,
monitor_all_ports_link_status, NULL);