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

[Yordan Karadzhov <y.karadz@gmail.com>]

On 19.01.22 г. 0:40 ч., Steven Rostedt wrote:
> 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).

Yes, but we still need to check the return value and print an error message in the case of a failure.

> 
>> 		}
>>
>>> +		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.

So far I'v been trying to print the error log every time when a libtracefs API returns an error. I know that not all of 
the errors are adding messages to the log, but I was thinking that this is something that is not guarantied to be stable 
and may change in the future.

Thanks!
Yordan

> 
>>
>>> +		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
>