| Message ID | 20240701115237.371020-1-quic_ekangupt@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v3] misc: fastrpc: Increase max user PD initmem size | expand |
On Mon, Jul 01, 2024 at 05:22:37PM +0530, Ekansh Gupta wrote: > For user PD initialization, initmem is allocated and sent to DSP for > initial memory requirements like shell loading. This size is passed > by user space and is checked against a max size. Why does fastrpc_init_create_process() allocate 4x the passed value and why is the value rounded up to INIT_FILELEN_MAX? > For unsigned PD > offloading requirement, more than 2MB size could be passed by user > which would result in PD initialization failure. Increase the maximum > size that can be passed py user for user PD initmem allocation. Sounds good, but why not 2.1MB or a rounder arbitrary value of 8 or 16? What is actually expected to fit in this initial memory? Is it the shell that has grown beyond 2MB? Also, s/py/by > Any > additional memory sent to DSP during PD init is used as the PD heap. > Does this mean that the initmem is used for shell loading and initial heap, and if more heap is needed after that the DSP can request more memory? Related to the question in v2, how is this memory accounted for? What would it mean that init.filefd != 0 in fastrpc_init_create_process(), will that pre-allocated memory (which was allocated without any size checks afaict) then be used for the same purpose? Why is a buffer of 4x the size of initfd then also allocated and mapped to the DSP? Could you please send a patch adding some comments documenting these things, the steps taken to create a new process, and what the 6 arguments built in fastrpc_init_create_process() actually means? Perhaps I'm just failing to read the answers already in the implementation, if so I'm sorry. Regards, Bjorn > Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP") > Cc: stable <stable@kernel.org> > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > --- > Changes in v2: > - Modified commit text. > - Removed size check instead of updating max file size. > Changes in v3: > - Added bound check again with a higher max size definition. > - Modified commit text accordingly. > > drivers/misc/fastrpc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 5204fda51da3..11a230af0b10 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -38,7 +38,7 @@ > #define FASTRPC_INIT_HANDLE 1 > #define FASTRPC_DSP_UTILITIES_HANDLE 2 > #define FASTRPC_CTXID_MASK (0xFF0) > -#define INIT_FILELEN_MAX (2 * 1024 * 1024) > +#define INIT_FILELEN_MAX (5 * 1024 * 1024) > #define INIT_FILE_NAMELEN_MAX (128) > #define FASTRPC_DEVICE_NAME "fastrpc" > > -- > 2.34.1 > > >
On 7/2/2024 2:44 AM, Bjorn Andersson wrote: > On Mon, Jul 01, 2024 at 05:22:37PM +0530, Ekansh Gupta wrote: >> For user PD initialization, initmem is allocated and sent to DSP for >> initial memory requirements like shell loading. This size is passed >> by user space and is checked against a max size. > Why does fastrpc_init_create_process() allocate 4x the passed value and > why is the value rounded up to INIT_FILELEN_MAX? The passed value is actually the size of fastrpc shell. This size is not sufficient for the user PD initialization and the PD also needs some additional memory for it's other requirements like heap. The value is aligned to 1 MB and there is a possibility that the user passed value is zero(user can only pass the size if it can open the shell). For this, there is at least some memory allocated and sent to DSP for PD initialization(2MB as of today). >> For unsigned PD >> offloading requirement, more than 2MB size could be passed by user >> which would result in PD initialization failure. Increase the maximum >> size that can be passed py user for user PD initmem allocation. > Sounds good, but why not 2.1MB or a rounder arbitrary value of 8 or 16? > > What is actually expected to fit in this initial memory? Is it the shell > that has grown beyond 2MB? I checked this again with the size of shell and I see that the shell size is not going beyond 2 MB, it's the unsigned PD requirement which is asking for more memory. This is because there are some static heap initialization specifically for unsigned PD. Do you suggest having a different definition for minimum initmem? Or have it as a local variable which changes based on PD type(2MB for signed PD and 5MB for unsigned PD)? > > Also, s/py/by > >> Any >> additional memory sent to DSP during PD init is used as the PD heap. >> > Does this mean that the initmem is used for shell loading and initial > heap, and if more heap is needed after that the DSP can request more > memory? Related to the question in v2, how is this memory accounted for? Yes this is for the initial heap requirement of PD(described above also). In case any more memory is required by DSP PD, it will make a reverse call to borrow more memory from HLOS using ADD_PAGES request which is supported by fastrpc_req_mmap. However, for unsigned PD the heaps are statically initialized which brings the requirement of some additional memory. > > What would it mean that init.filefd != 0 in > fastrpc_init_create_process(), will that pre-allocated memory (which was > allocated without any size checks afaict) then be used for the same > purpose? Why is a buffer of 4x the size of initfd then also allocated > and mapped to the DSP? The init.filefd is the FD of fastrpc shell that is opened and read by the process user space. I believe the pre-allocated memory you mentioned is the memory pointed by init.file. If the shell file is opened by user process DSP will load the shell and add the initmem to the DSP PD pool. If the user space has not opened and read the shell, DSP root PD daemon will open and read the shell for loading for PD spawning. Please let me know if there are any more questions here. Basically the usage of initmem is for shell loading and remaining memory is added to PD pool of heap and other usage. > > > Could you please send a patch adding some comments documenting these > things, the steps taken to create a new process, and what the 6 > arguments built in fastrpc_init_create_process() actually means? Sure, I'll add this information in the next spin. > > Perhaps I'm just failing to read the answers already in the > implementation, if so I'm sorry. Thanks for reviewing the patch. I'll add most of the information in commit text and as comments in the next patch. --Ekansh > > Regards, > Bjorn > >> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP") >> Cc: stable <stable@kernel.org> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> >> --- >> Changes in v2: >> - Modified commit text. >> - Removed size check instead of updating max file size. >> Changes in v3: >> - Added bound check again with a higher max size definition. >> - Modified commit text accordingly. >> >> drivers/misc/fastrpc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >> index 5204fda51da3..11a230af0b10 100644 >> --- a/drivers/misc/fastrpc.c >> +++ b/drivers/misc/fastrpc.c >> @@ -38,7 +38,7 @@ >> #define FASTRPC_INIT_HANDLE 1 >> #define FASTRPC_DSP_UTILITIES_HANDLE 2 >> #define FASTRPC_CTXID_MASK (0xFF0) >> -#define INIT_FILELEN_MAX (2 * 1024 * 1024) >> +#define INIT_FILELEN_MAX (5 * 1024 * 1024) >> #define INIT_FILE_NAMELEN_MAX (128) >> #define FASTRPC_DEVICE_NAME "fastrpc" >> >> -- >> 2.34.1 >> >> >>
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 5204fda51da3..11a230af0b10 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -38,7 +38,7 @@ #define FASTRPC_INIT_HANDLE 1 #define FASTRPC_DSP_UTILITIES_HANDLE 2 #define FASTRPC_CTXID_MASK (0xFF0) -#define INIT_FILELEN_MAX (2 * 1024 * 1024) +#define INIT_FILELEN_MAX (5 * 1024 * 1024) #define INIT_FILE_NAMELEN_MAX (128) #define FASTRPC_DEVICE_NAME "fastrpc"
For user PD initialization, initmem is allocated and sent to DSP for initial memory requirements like shell loading. This size is passed by user space and is checked against a max size. For unsigned PD offloading requirement, more than 2MB size could be passed by user which would result in PD initialization failure. Increase the maximum size that can be passed py user for user PD initmem allocation. Any additional memory sent to DSP during PD init is used as the PD heap. Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP") Cc: stable <stable@kernel.org> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> --- Changes in v2: - Modified commit text. - Removed size check instead of updating max file size. Changes in v3: - Added bound check again with a higher max size definition. - Modified commit text accordingly. drivers/misc/fastrpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)