Re: [PATCH v3] trace-cruncher: Add API to set tracing CPU affinity

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 18 Jan 2022 12:08:44 +0200
Yordan Karadzhov <y.karadz@gmail.com> wrote:

> On 18.01.22 г. 3:18 ч., Steven Rostedt wrote:

> > diff --git a/examples/reset-affinity.py b/examples/reset-affinity.py
> > new file mode 100755
> > index 0000000..2bca32e
> > --- /dev/null
> > +++ b/examples/reset-affinity.py
> > @@ -0,0 +1,5 @@
> > +#!/usr/bin/env python3
> > +
> > +import tracecruncher.ftracepy as ft
> > +
> > +ft.reset_affinity();
> > diff --git a/examples/test-affinity.py b/examples/test-affinity.py
> > new file mode 100755
> > index 0000000..1619931
> > --- /dev/null
> > +++ b/examples/test-affinity.py
> > @@ -0,0 +1,5 @@
> > +#!/usr/bin/env python3
> > +
> > +import tracecruncher.ftracepy as ft
> > +
> > +ft.set_affinity(cpus=["0-1","3"]);
> > diff --git a/src/ftracepy-utils.c b/src/ftracepy-utils.c
> > index a735d88..e42f69c 100644
> > --- a/src/ftracepy-utils.c
> > +++ b/src/ftracepy-utils.c
> > @@ -2394,6 +2394,99 @@ PyObject *PyFtrace_hist(PyObject *self, PyObject *args,
> >   	return NULL;
> >   }
> >   
> > +PyObject *PyFtrace_reset_affinity(PyObject *self, PyObject *args,
> > +				  PyObject *kwargs)
> > +{
> > +	struct tracefs_instance *instance;
> > +	static char *kwlist[] = {"instance", NULL};
> > +	PyObject *py_inst = NULL;
> > +	int ret;
> > +
> > +	if (!PyArg_ParseTupleAndKeywords(args,
> > +					 kwargs,
> > +					 "|O",
> > +					 kwlist,
> > +					 &py_inst)) {
> > +		return NULL;
> > +	}
> > +
> > +	if (!get_optional_instance(py_inst, &instance))
> > +		goto err;  
> 
> No need to have 'goto' in this function. Just return NULL (see also my comment at the bottom).
> 
> 
> > +
> > +	/* NULL for CPUs will set all available CPUS */
> > +	ret = tracefs_instance_set_affinity(instance, NULL);
> > +	if (ret < 0) {
> > +		PyErr_SetString(TRACECRUNCHER_ERROR,
> > +				"Failed to reset tracing affinity.");
> > +		goto err;
> > +	}  
> 
> Can we make the call of tracefs_instance_set_affinity(), the error check and the error message a static helper?
> I will explain why bellow in the patch.
> 
> > +
> > +	Py_RETURN_NONE;
> > +err:
> > +	return NULL;
> > +}
> > +
> > +PyObject *PyFtrace_set_affinity(PyObject *self, PyObject *args,
> > +						PyObject *kwargs)
> > +{
> > +	struct tracefs_instance *instance;
> > +	static char *kwlist[] = {"cpus", "instance", NULL};
> > +	PyObject *py_cpus;
> > +	PyObject *py_inst = NULL;
> > +	const char *cpu_str;
> > +	int ret;
> > +
> > +	if (!init_print_seq())
> > +		return false;  
> 
> We have to return NULL here.

Oops, silly cut and paste issue.

> 
> > +
> > +	if (!PyArg_ParseTupleAndKeywords(args,
> > +					 kwargs,
> > +					 "O|O",
> > +					 kwlist,
> > +					 &py_cpus,
> > +					 &py_inst)) {
> > +		return NULL;
> > +	}
> > +
> > +	if (PyUnicode_Check(py_cpus)) {
> > +		cpu_str = (const char *)PyUnicode_DATA(py_cpus);  
> 
> And here we can have
> 
> 		if ((is_all(cpu_str) {
> 			Call the static helper for 'reset' here and return.

Do we need a static helper? It is simply:

	tracefs_instance_set_affinity(instance, NULL);

and that will set all of them. (NULL for CPUs means all CPUs).

> 		}
> 
> > +		if (trace_seq_puts(&seq, cpu_str) < > +			goto err_seq;
> > +	} else if (PyList_CheckExact(py_cpus)) {
> > +		int i, n = PyList_Size(py_cpus);
> > +
> > +		for (i = 0; i < n; ++i) {
> > +			cpu_str = str_from_list(py_cpus, i);
> > +			if (i)
> > +				trace_seq_putc(&seq, ',');
> > +			if (trace_seq_puts(&seq, cpu_str) < 0)
> > +				goto err_seq;
> > +		}
> > +	} else {
> > +		PyErr_SetString(TRACECRUNCHER_ERROR,
> > +				"Invalid \"cpus\" type.");  
> 
> It may be better to use TfsError_fmt() here.
> This helper will print also the error log of libtracefs.
> The same comment applies for all error messages below.

Do we want that as the error log will not be modified by this operation.

> 
> > +		goto err;  
>   		return NULL;
> > +	}
> > +
> > +	if (!get_optional_instance(py_inst, &instance))
> > +		goto err;  
> 		return NULL;
> > +
> > +	trace_seq_terminate(&seq);
> > +	ret = tracefs_instance_set_affinity(instance, seq.buffer);
> > +	if (ret < 0) {
> > +		PyErr_SetString(TRACECRUNCHER_ERROR,
> > +				"Invalid \"cpus\" argument.");
> > +		goto err;  
>   		return NULL;
> > +	}
> > +
> > +	Py_RETURN_NONE;
> > +err_seq:
> > +	PyErr_SetString(TRACECRUNCHER_ERROR,
> > +			"Internal processing string error.");
> > +err:
> > +	return NULL;  
> 
> I guess you use 'goto err;' everywhere, because you want to stress that returning NULL is essentially exiting with 
> error. However, so far we used just 'return NULL' everywhere and I would prefer to keep the code consistent.
> Maybe in a separate patch we can replace all ''return NULL;' lines with a macro that can have some self-explanatory name?

Yeah, I was trying to match the existing code, as I'm new to this. ;-)

-- Steve