Re: [PATCH v8 1/8] x86/resctrl: Add multiple tasks to the resctrl group at once

[Fenghua Yu]

Hi, Babu,

On 9/7/23 07:44, Moger, Babu wrote:
> Hi Fenghua,
>
> On 9/6/2023 3:42 PM, Fenghua Yu wrote:
> > Hi, Babu,
> >
> > On 9/6/23 07:56, Moger, Babu wrote:
> > > > > @@ -715,7 +714,27 @@ static ssize_t rdtgroup_tasks_write(struct 
> > > > > kernfs_open_file *of,
> > > > >           goto unlock;
> > > > >       }
> > > > >   -    ret = rdtgroup_move_task(pid, rdtgrp, of);
> > > > > +    while (buf && buf[0] != '\0' && buf[0] != '\n') {
> > > > > +        pid_str = strim(strsep(&buf, ","));
> > > > > +
> > > > > +        if (kstrtoint(pid_str, 0, &pid)) {
> > > > > +            rdt_last_cmd_puts("Task list parsing error\n");
> > > >
> > > > It would be better to show the failed pid string in the failure report:
> > > > +            rdt_last_cmd_puts("Task list parsing error pid %s\n", 
> > > > pid_str);
> > > >
> > > > So user will know which pid string causes the failure?
> > >
> > > It was already discussed. Printing the characters during parsing 
> > > error may not be much useful.
> >
> > Could you please let me know where printing "pid_str" is discussed?
>
> My bad.  Should have read your comments more carefully.

Thank you for your clarification.

> > My understanding is a similar thing is discussed in v3:
> > https://lore.kernel.org/all/167778866506.1053859.2329229096484796501.stgit@bmoger-ubuntu/ 
> >
> >
> > Then v4 has this code without printing pid_str. In v4, there is a 
> > similar discussion of printing pid, but not pid_str.
> >
> > But I cannot find a discussion of why "pid_str" is not printed.
> >
> > If kstritoint(pid_str, 0, &pid) fails, without printing pid_str, how 
> > can user know which pid string fails? e.g. user tries to move 100 pids 
> > and the 51st pid parsing fails. It's hard for user to know the 51st 
> > pid fails without showing pid_str in the error info and then it's hard 
> > for the user to decide to re-do moving or aborting moving etc.
>
> That is correct.  Will add following print statement o print the pid_str.
>
> rdt_last_cmd_printf("Task list parsing error pid %s\n", pid_str);

Great!

Thanks.

-Fenghua