Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pg/mod config v1 #2157

Closed

Conversation

pierregondois
Copy link
Contributor

No description provided.

lisa/_assets/kmodules/lisa/fs.c Outdated Show resolved Hide resolved
lisa/_assets/kmodules/lisa/fs.c Outdated Show resolved Hide resolved
@chrisswinchatt-arm
Copy link

I haven't fully reviewed this but just noticed a couple of things.

Copy link
Collaborator

@bea-arm bea-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a partial review, mainly of vfs interface. Will try to get back to the rest soon.

lisa/_assets/kmodules/lisa/fs.c Show resolved Hide resolved
lisa/_assets/kmodules/lisa/fs.c Outdated Show resolved Hide resolved
lisa/_assets/kmodules/lisa/fs.c Outdated Show resolved Hide resolved
lisa/_assets/kmodules/lisa/fs.c Show resolved Hide resolved
lisa/_assets/kmodules/lisa/fs.c Outdated Show resolved Hide resolved
struct feature *feature;

for_each_feature(feature) {
if (!strcmp(feature->name, val->data))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look safe when the param value is not a pointer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the case, is the concern about having val->data not being a string, but a data of another type (like an integer for instance) ?

The input val should have been parsed as a string, cf.:

struct feature_param lisa_features_param = {
	.name = "lisa_features_param",
       [...]
	.ops = &feature_param_ops_string,
	.validate = feature_param_lisa_validate,
       [...]
};

lisa/_assets/kmodules/lisa/fs.c Outdated Show resolved Hide resolved
lisa/_assets/kmodules/lisa/fs.c Outdated Show resolved Hide resolved
lisa/_assets/kmodules/lisa/fs.c Outdated Show resolved Hide resolved

val = hlist_entry(v, struct feature_param_entry_value, node);

if (param->ops->stringify) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the value is already a string ? Seems this is no being shown then ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strings currently have their .stringify() cb (cf. below).

const struct feature_param_ops feature_param_ops_string = {
	.set = feature_param_set_string,
	.stringify = feature_param_stringify_string,
	.is_equal = feature_param_is_equal_string,
	.copy = feature_param_copy_string,
};

I agree it's not optimal, I'll rework it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's convenient to just have just one common case. I'll let you decide if a more optimal solution is required to handle strings.

Add list_count_elements() utility function.

Original-author: Beata Michalska <[email protected]>
Signed-off-by: Pierre Gondois <[email protected]>
In order to configure lisa features and their parameters at
runtime, add support for a VFS in the lisa module.

The lisa VFS can be mounted as:
$ mkdir /lisa
$ mount -t lisa none /lisa

It contains the following files/folders:
lisa_fs
\-activate                  # Activate this config
\-available_features        # List of available features
\-event__lisa__perf_counter # (E.g.) Dir to configure perf_counter feature
  \-generic_counters        # Generic counter to track
  \-pmu_raw_counters        # PMU raw counters to track
\-configs                   # Allow multiple configurations
  \-custom_conf_0           # Custom configuration ()
    \- ...                  # Similar files as in lisa_fs/
\-set_features              # Features to enable

A feature can have various parameters. For instance, the
'event__lisa__perf_counter' feature has two parameters that can
be configured: 'generic_counters', 'pmu_raw_counters'.

Multiple configurations can co-exist. The 'main' configuration is shown
above, and other custom configurations can be created in the 'configs'
directory.
If multiple configurations are active simultaneously, the VFS merges
them into one global configuration. For instance, if:
- feat_0 is set in custom_conf_0
- feat_1 is set in custom_conf_1
and both configurations are active, the resulting configuration will
have feat_0 and feat_1 active.

Original-author: Beata Michalska <[email protected]>
Signed-off-by: Pierre Gondois <[email protected]>
…aram

Features are declared/defined using macros. Update them to allow
declaring feature parameters along features.

Original-author: Beata Michalska <[email protected]>
Signed-off-by: Pierre Gondois <[email protected]>
A previous patch adds support for a virtual file system (VFS)
for the lisa module. This VFS allows to configure the module
features at runtime.
Make use of this VFS to configure the desired features. Features
that have parameters can be configured from a notebook with:
"""
features = {
  "lisa__perf_counter": {
    "generic_counters":
      ["cpu_cycles", "l1d_cache", "inst_retired"]
  }
}
ftrace_coll = FtraceCollector(target, ..., kmod_features=features)
"""

Original-author: Beata Michalska <[email protected]>
Signed-off-by: Pierre Gondois <[email protected]>
The lisa module was previously configured at load time by
providing module parameters. The newly added VFS interface
allows to configure kmod features at runtime.
Remove the kmod parameters generation.

Original-author: Beata Michalska <[email protected]>
Signed-off-by: Pierre Gondois <[email protected]>
@douglas-raillard-arm
Copy link
Contributor

Closing as the project has moved to https://gitlab.arm.com/tooling/lisa/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants