| Message ID | 20210323012755.155237800@goodmis.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | libtracfes: Add tracefs_function_filter() | expand |
On Mon, 22 Mar 2021 21:27:55 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > This adds a new API tracefs_function_filter() as described in: > > https://bugzilla.kernel.org/show_bug.cgi?id=210643 > > It will use regular expressions against available_filter_functions (or even > the kernel glob expression) to enable the function by the index method if it > is supported. If it is not supported, it will go back to the writing of the > filter strings directly into the set_ftrace_filter file. > > Playing with the interface some more, I feel it's not quite adequate. The returning the negative number of filters that failed, isn't very useful. Having the errs array that points to those filters gives us that information. Also, because of the way that file works in the kernel, we need to be able to call this function several times without closing the file. That's because the actions take place when the file is closed, *not* when a write is made. Having the interface return errors without closing the file would allow the user to fix the failed filters and try again. I plan on committing this patch series, so any new changes will be done on top of them. But here's what I think needs to be done to make the interface more usable. - Create a tracefs_function_filter_commit(instance) API that will close the file and commit the changes. - Have the tracefs_function_filter() open the set_ftrace_filter file if it is not already opened. This will allow for the function to be called multiple times. The file descriptor will be part of the instance descriptor or a global variable for the top level instance. The opening of the files will need to be protected by a pthread mutex. Note, the "reset" parameter is only applicable if the file is not already opened. - On success, return 0 and tracefs_function_filter_commit() must be called. - If the file descriptor is opened and an error happens, return 1, and the tracefs_function_filter_commit() still needs to be called, but the user can call tracefs_function_filter() again to try to fix the problem. - If an error is found before the file descriptor is opened, then tracefs_function_filter_commit() does not need to be called. That is: ret = tracefs_function_filter(...); if (ret >= 0) tracefs_function_filter_commit(); - Now for errs, it will be allocated if a problem happened on a filter, and may be set if the return value is non zero (1 or -1). The user can use it to know if the problem happened on a filter as well as find out which filter had the problem. - We can have a tracefs_read_function_filter() that returns an array of strings which ends with a NULL pointer (and needs to be freed with tracefs_list_free(), and have this: save_filters = tracefs_read_function_filter(instance); ret = tracefs_function_filter(instance, filters, NULL, true, &errs); if (ret > 0 && errs) { /* Modified but failed */ int i, j; for (i = 0; filters[i]; i++) ; for (j = 0; errs[i]; j++) ; if (i == j) /* all filters failed! Put back the original */ tracefs_function_filter(instance, save_filters, NULL, false, NULL); } if (ret >= 0) tracefs_function_filter_commit(instance); Another reason to have it not close the file is because we only support passing in one module at a time. If the user wants to enable functions in two modules, they would need to call this twice, and we want them to be able to do so and have both changes take affect at the same time. Thoughts? -- Steve
Hi steve, I have doubt On Tue, Mar 23, 2021 at 6:22 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Mon, 22 Mar 2021 21:27:55 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > This adds a new API tracefs_function_filter() as described in: > > > > https://bugzilla.kernel.org/show_bug.cgi?id=210643 > > > > It will use regular expressions against available_filter_functions (or even > > the kernel glob expression) to enable the function by the index method if it > > is supported. If it is not supported, it will go back to the writing of the > > filter strings directly into the set_ftrace_filter file. > > > > > > Playing with the interface some more, I feel it's not quite adequate. > > The returning the negative number of filters that failed, isn't very > useful. Having the errs array that points to those filters gives us that > information. > > Also, because of the way that file works in the kernel, we need to be able > to call this function several times without closing the file. That's > because the actions take place when the file is closed, *not* when a write > is made. Having the interface return errors without closing the file would > allow the user to fix the failed filters and try again. > > I plan on committing this patch series, so any new changes will be done on > top of them. But here's what I think needs to be done to make the interface > more usable. > > - Create a tracefs_function_filter_commit(instance) API that will close > the file and commit the changes. > > - Have the tracefs_function_filter() open the set_ftrace_filter file if it > is not already opened. This will allow for the function to be called > multiple times. The file descriptor will be part of the instance > descriptor or a global variable for the top level instance. The opening > of the files will need to be protected by a pthread mutex. > Note, the "reset" parameter is only applicable if the file is not > already opened. > > - On success, return 0 and tracefs_function_filter_commit() must be called. > > - If the file descriptor is opened and an error happens, return 1, and the > tracefs_function_filter_commit() still needs to be called, but the user > can call tracefs_function_filter() again to try to fix the problem. > > - If an error is found before the file descriptor is opened, then > tracefs_function_filter_commit() does not need to be called. > > That is: > > ret = tracefs_function_filter(...); > if (ret >= 0) > tracefs_function_filter_commit(); > > - Now for errs, it will be allocated if a problem happened on a filter, > and may be set if the return value is non zero (1 or -1). The user can > use it to know if the problem happened on a filter as well as find out > which filter had the problem. > > - We can have a tracefs_read_function_filter() that returns an array of > strings which ends with a NULL pointer (and needs to be freed with > tracefs_list_free(), and have this: > > save_filters = tracefs_read_function_filter(instance); are we going to use the tracefs_read_function_filter() function to read the set_ftrace_file for getting written filters? > > ret = tracefs_function_filter(instance, filters, NULL, true, &errs); > if (ret > 0 && errs) { > /* Modified but failed */ > int i, j; > for (i = 0; filters[i]; i++) > ; > for (j = 0; errs[i]; j++) > ; > if (i == j) > /* all filters failed! Put back the original */ > tracefs_function_filter(instance, save_filters, NULL, false, NULL); > } > if (ret >= 0) > tracefs_function_filter_commit(instance); > > Another reason to have it not close the file is because we only support > passing in one module at a time. If the user wants to enable functions in > two modules, they would need to call this twice, and we want them to be > able to do so and have both changes take affect at the same time. > > Thoughts? > > -- Steve Thanks, sameer.
On Wed, 24 Mar 2021 15:43:28 +0530 Sameeruddin Shaik <sameeruddin.shaik8@gmail.com> wrote: > > - We can have a tracefs_read_function_filter() that returns an array of > > strings which ends with a NULL pointer (and needs to be freed with > > tracefs_list_free(), and have this: > > > > save_filters = tracefs_read_function_filter(instance); > > are we going to use the tracefs_read_function_filter() function to > read the set_ftrace_file for getting written filters? Yes, why not? But it would not return probes, only filters. That is, the set_ftrace_filter also shows probes, and they are different. But the tracefs_read_function_filter() should not return them. For them, we should have a tracefs_read_function_probes() API. -- Steve > > > > ret = tracefs_function_filter(instance, filters, NULL, true, &errs); > > if (ret > 0 && errs) { > > /* Modified but failed */ > > int i, j; > > for (i = 0; filters[i]; i++) > > ; > > for (j = 0; errs[i]; j++) > > ; > > if (i == j) > > /* all filters failed! Put back the original */ > > tracefs_function_filter(instance, save_filters, NULL, false, NULL); > > }
On Fri, 26 Mar 2021 05:55:46 +0530 sameeruddin shaik <sameeruddin.shaik8@gmail.com> wrote: > > On 23/03/21 6:22 pm, Steven Rostedt wrote: > > On Mon, 22 Mar 2021 21:27:55 -0400 > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > >> This adds a new API tracefs_function_filter() as described in: > >> > >> https://bugzilla.kernel.org/show_bug.cgi?id=210643 > >> > >> It will use regular expressions against available_filter_functions (or even > >> the kernel glob expression) to enable the function by the index method if it > >> is supported. If it is not supported, it will go back to the writing of the > >> filter strings directly into the set_ftrace_filter file. > >> > >> > > Playing with the interface some more, I feel it's not quite adequate. > > > > The returning the negative number of filters that failed, isn't very > > useful. Having the errs array that points to those filters gives us that > > information. > Yeah, Anyway we are pointing the failed filters using the errs pointer. > But to differentiate > > the failed filters from the general errors, we can return -1, what you say? > No, I was thinking that -1 means that the file was never opened, but 1 means an error happened after the file was opened (and thus it must be closed, or "committed"). In either case, the errs can be set. I have the function checking the available_filter_functions to see if there's any matches before it even opens the file, and if any filter fails, I was thinking it should return -1 (meaning failure before opening) and the errs will be set to the filters that failed (if it was due to a filter failing. We should clear errno, at the start, so that if a filter failed, errno is zero, and errs points to the failed filters, if there was another failure, then errno would be set instead). If all filters match, and we use your old method and there's some other kind of error in writing the filter to set_ftrace_filter, then we could have errs point to that filter, but still return 1 because the file was opened and it still needs to be closed / committed. Note, this wont happen on kernels 5.1 and beyond, as they support indexing, and once we get to the indexing part, errs wont be used. -- Steve
On 23/03/21 6:22 pm, Steven Rostedt wrote: > On Mon, 22 Mar 2021 21:27:55 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > >> This adds a new API tracefs_function_filter() as described in: >> >> https://bugzilla.kernel.org/show_bug.cgi?id=210643 >> >> It will use regular expressions against available_filter_functions (or even >> the kernel glob expression) to enable the function by the index method if it >> is supported. If it is not supported, it will go back to the writing of the >> filter strings directly into the set_ftrace_filter file. >> >> > Playing with the interface some more, I feel it's not quite adequate. > > The returning the negative number of filters that failed, isn't very > useful. Having the errs array that points to those filters gives us that > information. Yeah, Anyway we are pointing the failed filters using the errs pointer. But to differentiate the failed filters from the general errors, we can return -1, what you say? > > Also, because of the way that file works in the kernel, we need to be able > to call this function several times without closing the file. That's > because the actions take place when the file is closed, *not* when a write > is made. Having the interface return errors without closing the file would > allow the user to fix the failed filters and try again. > > I plan on committing this patch series, so any new changes will be done on > top of them. But here's what I think needs to be done to make the interface > more usable. > > - Create a tracefs_function_filter_commit(instance) API that will close > the file and commit the changes. > > - Have the tracefs_function_filter() open the set_ftrace_filter file if it > is not already opened. This will allow for the function to be called > multiple times. The file descriptor will be part of the instance > descriptor or a global variable for the top level instance. The opening > of the files will need to be protected by a pthread mutex. > Note, the "reset" parameter is only applicable if the file is not > already opened. > > - On success, return 0 and tracefs_function_filter_commit() must be called. > > - If the file descriptor is opened and an error happens, return 1, and the > tracefs_function_filter_commit() still needs to be called, but the user > can call tracefs_function_filter() again to try to fix the problem. > > - If an error is found before the file descriptor is opened, then > tracefs_function_filter_commit() does not need to be called. > > That is: > > ret = tracefs_function_filter(...); > if (ret >= 0) > tracefs_function_filter_commit(); > > - Now for errs, it will be allocated if a problem happened on a filter, > and may be set if the return value is non zero (1 or -1). The user can > use it to know if the problem happened on a filter as well as find out > which filter had the problem. > > - We can have a tracefs_read_function_filter() that returns an array of > strings which ends with a NULL pointer (and needs to be freed with > tracefs_list_free(), and have this: > > save_filters = tracefs_read_function_filter(instance); > > ret = tracefs_function_filter(instance, filters, NULL, true, &errs); > if (ret > 0 && errs) { > /* Modified but failed */ > int i, j; > for (i = 0; filters[i]; i++) > ; > for (j = 0; errs[i]; j++) > ; > if (i == j) > /* all filters failed! Put back the original */ > tracefs_function_filter(instance, save_filters, NULL, false, NULL); > } > if (ret >= 0) > tracefs_function_filter_commit(instance); > > Another reason to have it not close the file is because we only support > passing in one module at a time. If the user wants to enable functions in > two modules, they would need to call this twice, and we want them to be > able to do so and have both changes take affect at the same time. > > Thoughts? > > -- Steve
This adds a new API tracefs_function_filter() as described in: https://bugzilla.kernel.org/show_bug.cgi?id=210643 It will use regular expressions against available_filter_functions (or even the kernel glob expression) to enable the function by the index method if it is supported. If it is not supported, it will go back to the writing of the filter strings directly into the set_ftrace_filter file. Sameeruddin shaik (1): libtracefs: An API to set the filtering of functions Steven Rostedt (VMware) (6): libtracefs: Move opening of file out of controlled_write() libtracefs: Add add_errors() helper function for control_write() libtracefs: Add checking of available_filter_functions to tracefs_function_filter() libtracefs: Add write_filter() helper function libtracefs: Allow for setting filters with regex expressions libtracefs: Add indexing to set functions in tracefs_function_filter() ---- include/tracefs.h | 3 +- src/tracefs-tools.c | 521 +++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 522 insertions(+), 2 deletions(-)