> -----Original Message----- > From: ext Maxim Uvarov [mailto:[email protected]] > Sent: Wednesday, September 03, 2014 1:37 PM > To: Savolainen, Petri (NSN - FI/Espoo); [email protected] > Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag > > On 09/03/2014 11:36 AM, Savolainen, Petri (NSN - FI/Espoo) wrote: > > > >> -----Original Message----- > >> From: [email protected] [mailto:lng-odp- > >> [email protected]] On Behalf Of ext Maxim Uvarov > >> Sent: Tuesday, September 02, 2014 6:43 PM > >> To: [email protected] > >> Subject: Re: [lng-odp] [PATCH 1/2] Shared memory reserve flag > >> > >> > >> > >> On 09/01/2014 03:53 PM, Petri Savolainen wrote: > >>> Add flags parameter to reserve call. > >>> * SW_ONLY: optimize memory usage when HW accelerators are not > >>> involved. > >>> * PROC: for communication with other (non-ODP) processes in the > >>> system > >>> * KERNEL: for communication with a kernel (OS) driver > >> Hi Petri, > >> > >> this patch is about the same which I sent. > > Didn't realize that. This is not only for IPC, but the shm flag was on > TODO list from last sprint to help Taras to optimize shared memory > allocation (flag SW only vs. SW+HW accelerator access). > > Yes, it's patch for reserving memory. Because it's general api I need > the same for IPC. > My patch was here: > https://patches.linaro.org/35037/ > > > > >> 1. One difference is KERNEL flag. How if KERNEL flag will be used? Is > >> there is specific need for that? What will happen if > >> app will set this flag but flag is not supported by platform. > > I was thinking to use that to flag memory that will be shared with a > kernel driver (but not with external processes). It can be dropped now if > not needed. Flags are additional information to the implementation what's > the intended memory usage, so that they can optimize how HW resource are > used. > > Lets do that. Drop this flag for now. And if Taras needs this flag he > will add it with KS2 implementation. > >> 2. If public API changed then you need to fix all existence platforms, > >> to not break their compilation/work. > > I changed the reference API and implementation (== linux-generic). > Maintainers of other implementations are free to decide when (and more > importantly who) to follow. We cannot keep all implementations in 100% > sync. It's better to break a build (if they are reusing linux-generic > code), than try to fix all implementations at once (with potentially > untested, buggy code). > > You should not break other platforms when changing api for > linux-generic. You added new flag to odp_shm_reserve() for example to > example/generator/odp_generator.c but ks2 and dpdk still have on one > argument less. So their compilation will fail. And this things have to > be avoided. We also decided that git history > have to be 'git bisect' friendly. That rule is also acceptable on > other arches.
Maybe we need to start versioning the API and test apps, so that it's possible to have different implementations out-of-sync. Otherwise after couple of more platforms, it's pretty impossible to modify the (reference) API if all implementations must be modified at once. Who can master HW/SDK/implementation details of all the platforms? Also it would require pretty tight lock-step development (not very agile). A new argument is trivial to add... but if implementation is missing, test apps would fail still... if you implement you should test it... does everybody have access to all HW platforms... -Petri > > >> 3. What I don't like in current odp_shm_reserve() is that it's > >> difficult to understand what is the reason of the error. > >> I think it's reasonable to change that function from: > >> void *odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > >> odp_shm_e flag) > >> to > >> int odp_shm_reserve(void **addr, const char *name, uint64_t size, > >> uint64_t align, > >> odp_shm_e flag) > >> > >> where return codes are: 1) chunk with that name already exist. addr is > >> set. 2) mmap failed 3) exceed number of ODP_SHM_NUM_BLOCKS and etc. > >> I.e. not just fail with ODP_ERR() but return some reasonable value. > > This goes back to errno discussion... I think it should return NULL and > set errno. Errno/abort()/etc changes will be done in sync with all other > APIs. > errno is ok for me. > > Maxim. > > -Petri > > > > _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
