Re: [PATCH v3] perf test record.sh: Raise limit of open file descriptors

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Wed, Aug 14, 2024 at 06:04:27PM +0200, Veronika Molnarova wrote:
> 
> 
> On 8/14/24 17:56, Arnaldo Carvalho de Melo wrote:
> > On Wed, Aug 14, 2024 at 12:54:51PM -0300, Arnaldo Carvalho de Melo wrote:
> >> On Wed, Aug 14, 2024 at 05:42:01PM +0200, Veronika Molnarova wrote:
> >>>
> >>>
> >>> On 8/14/24 17:35, Arnaldo Carvalho de Melo wrote:
> >>>> On Wed, Aug 14, 2024 at 12:32:39PM -0300, Arnaldo Carvalho de Melo wrote:
> >>>>> On Wed, Aug 14, 2024 at 05:17:34PM +0200, vmolnaro@redhat.com wrote:
> >>>>>> From: Veronika Molnarova <vmolnaro@redhat.com>
> >>>>>
> >>>>> Ok? I'm applying it with this change. Thanks,
> >>>>
> >>>> So I added this to the log message:
> >>>>
> >>>> Committer notes:
> >>>>
> >>>> Instead of disabling ShellCheck warnings all the uses of 'uname -n',
> >>>> i.e. those:
> >>>>
> >>>>   In tests/shell/record.sh line 35:
> >>>>   default_fd_limit=$(ulimit -Sn)
> >>>>                             ^-^ SC3045 (warning): In POSIX sh, ulimit -S is undefined.
> >>>>
> >>>> We can just switch from using '/bin/sh' to '/bin/bash' for this test, as
> >>>> bash _has_ 'ulimit -n', so ShellCheck will not emit that warning.
> >>>>
> >>>> There are dozens of 'perf test' shell tests that do just that,
> >>>> '/bin/bash' is a reasonable expectation for those tests.
> >>>>
> >>>> ----------------------------------------------------------
> >>>>
> >>>> Please let me know if you find any issues with this course of action,
> >>>>
> >>>> Thanks,
> >>>>
> >>>> - Arnaldo
> >>>>
> >>> If so then the check whether the ulimit is supported doesn't need to be done
> >>> as bash is given as a requirement. Thought that it should be supporting all
> >>> possible shells, even though couldn't find shell not supporting 'ulimit -Sn'.
> >>>
> >>> Can I send a quick fix that just changes to the '/bin/bash' so that the code
> >>> won't have unnecessary code?
> >>
> >> See my last message, I went with your v2 + switch from /bin/sh to
> >> /bin/bash, as you suggest above.
> > 
> > And added the '-S' to ulimit, since you changed that from v2, are you
> > sure that is better than using just -n? Why?
> > 
> > - Arnaldo
> > 
> 
> It doesn't matter for checking the value, 'ulimit -n' is the same as 'ulimit -Sn'.
> But when setting up the value without the option -S, both soft and hard limits are
> set to the provided value. For us, only raising the soft limit is important, as it
> is very unlikely that that hard limit would need to be raised. Also, then the hard
> limit would have to be restored separately to its original value.

Thanks for the explanation, I kept the -S.

- Arnaldo