Message ID | 56669862.9000509@linaro.org |
---|---|
State | New |
Headers | show |
On 12/08/2015 13:09, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >> -----Original Message----- >> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >> Sent: Tuesday, December 08, 2015 10:44 AM >> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [API-NEXT PATCHv2] linux-generic: tm: use >> odp_hash_crc32c() api to avoid arch issues >> >> On 12/08/2015 10:50, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>> Bill, >>> >>> hash_tbl_entry is now uintptr_t, which is 64 bits on a 64 bit build and >> 32 bits on a 32 bit build. These shifts and all other lines that expect >> that hash_tbl_entry (pointer size) is 64 bits should be updated also. >>> -Petri >>> >> Yes, that is what I said before. For now we can go with simple using 64 >> bit hash entries for 32 bit case also. >> Where 6 upper bits will be for defining first or secondary hash and it's >> size. And all lower 32 for address pointer. >> I.e. 26 bits will be lost. >> >> #define PRIMARY_HASH_TBL_SIZE (16 * 1024) >> 26 * PRIMARY_HASH_TBL_SIZE = 425984 = 53248 bytes = 13 pages; >> >> #define SECONDARY_HASH_TBL_SIZE 128 >> 26 * SECONDARY_HASH_TBL_SIZE = 3328 = 416 bytes >> >> So we lost for 32 bits some memory but code has to work in the same way >> as for 64 bits. >> After that if memory efficient fix will be needed for somebody than he >> can fix it. >> Or we will reuse some odp hash table api instead of odp_name_table.c. >> >> So for now I would do quick fix with some comment that there is unused >> memory for 32 bit case. >> >> --- a/platform/linux-generic/odp_name_table.c >> +++ b/platform/linux-generic/odp_name_table.c >> @@ -93,7 +93,7 @@ typedef struct { >> * NOT zero then this values points to a (linked list of) >> name_table_entry_t >> * records AND the bottom 6 bits are the length of this list. >> */ >> -typedef uintptr_t hash_tbl_entry_t; >> +typedef uint64_t hash_tbl_entry_t; >> >> >> BR, >> Maxim. > > odp_name_table.c: In function 'make_hash_tbl_entry': > odp_name_table.c:570:20: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] > hash_tbl_entry = (hash_tbl_entry_t)name_tbl_entry; > > > static hash_tbl_entry_t make_hash_tbl_entry(name_tbl_entry_t *name_tbl_entry, > uint32_t entry_cnt) > { > hash_tbl_entry_t hash_tbl_entry; > uint32_t new_entry_cnt; > > new_entry_cnt = MIN(entry_cnt + 1, 0x3F); > hash_tbl_entry = (hash_tbl_entry_t)(uintptr_t)name_tbl_entry; > hash_tbl_entry &= ~0x3F; > hash_tbl_entry |= new_entry_cnt; > return hash_tbl_entry; > } > > > And compiler is still happy with the casts? Maybe the casting issue was actually caused by removal of the uintptr_t cast above. > > -Petri > Petri, Looks like you did not apply that patch. If you apply it and do change to uint64_t everything compiles. Maxim.
The cast plus the uint64_t typedef is the v1 patch. So is that one now good? On Tue, Dec 8, 2015 at 4:45 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 12/08/2015 13:09, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >> >> -----Original Message----- >>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >>> Sent: Tuesday, December 08, 2015 10:44 AM >>> To: Savolainen, Petri (Nokia - FI/Espoo); lng-odp@lists.linaro.org >>> Subject: Re: [lng-odp] [API-NEXT PATCHv2] linux-generic: tm: use >>> odp_hash_crc32c() api to avoid arch issues >>> >>> On 12/08/2015 10:50, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>> >>>> Bill, >>>> >>>> hash_tbl_entry is now uintptr_t, which is 64 bits on a 64 bit build and >>>> >>> 32 bits on a 32 bit build. These shifts and all other lines that expect >>> that hash_tbl_entry (pointer size) is 64 bits should be updated also. >>> >>>> -Petri >>>> >>>> Yes, that is what I said before. For now we can go with simple using 64 >>> bit hash entries for 32 bit case also. >>> Where 6 upper bits will be for defining first or secondary hash and it's >>> size. And all lower 32 for address pointer. >>> I.e. 26 bits will be lost. >>> >>> #define PRIMARY_HASH_TBL_SIZE (16 * 1024) >>> 26 * PRIMARY_HASH_TBL_SIZE = 425984 = 53248 bytes = 13 pages; >>> >>> #define SECONDARY_HASH_TBL_SIZE 128 >>> 26 * SECONDARY_HASH_TBL_SIZE = 3328 = 416 bytes >>> >>> So we lost for 32 bits some memory but code has to work in the same way >>> as for 64 bits. >>> After that if memory efficient fix will be needed for somebody than he >>> can fix it. >>> Or we will reuse some odp hash table api instead of odp_name_table.c. >>> >>> So for now I would do quick fix with some comment that there is unused >>> memory for 32 bit case. >>> >>> --- a/platform/linux-generic/odp_name_table.c >>> +++ b/platform/linux-generic/odp_name_table.c >>> @@ -93,7 +93,7 @@ typedef struct { >>> * NOT zero then this values points to a (linked list of) >>> name_table_entry_t >>> * records AND the bottom 6 bits are the length of this list. >>> */ >>> -typedef uintptr_t hash_tbl_entry_t; >>> +typedef uint64_t hash_tbl_entry_t; >>> >>> >>> BR, >>> Maxim. >>> >> >> odp_name_table.c: In function 'make_hash_tbl_entry': >> odp_name_table.c:570:20: error: cast from pointer to integer of different >> size [-Werror=pointer-to-int-cast] >> hash_tbl_entry = (hash_tbl_entry_t)name_tbl_entry; >> >> >> static hash_tbl_entry_t make_hash_tbl_entry(name_tbl_entry_t >> *name_tbl_entry, >> uint32_t entry_cnt) >> { >> hash_tbl_entry_t hash_tbl_entry; >> uint32_t new_entry_cnt; >> >> new_entry_cnt = MIN(entry_cnt + 1, 0x3F); >> hash_tbl_entry = (hash_tbl_entry_t)(uintptr_t)name_tbl_entry; >> hash_tbl_entry &= ~0x3F; >> hash_tbl_entry |= new_entry_cnt; >> return hash_tbl_entry; >> } >> >> >> And compiler is still happy with the casts? Maybe the casting issue was >> actually caused by removal of the uintptr_t cast above. >> >> -Petri >> >> Petri, Looks like you did not apply that patch. If you apply it and do > change to uint64_t everything compiles. > > Maxim. > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 12/08/2015 14:27, Bill Fischofer wrote: > The cast plus the uint64_t typedef is the v1 patch. So is that one > now good? For me v1 was good if I remember. And v2 with that fix also is good. Petri do you have any notes? Maxim. > > On Tue, Dec 8, 2015 at 4:45 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 12/08/2015 13:09, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > > -----Original Message----- > From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>] > Sent: Tuesday, December 08, 2015 10:44 AM > To: Savolainen, Petri (Nokia - FI/Espoo); > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > Subject: Re: [lng-odp] [API-NEXT PATCHv2] linux-generic: > tm: use > odp_hash_crc32c() api to avoid arch issues > > On 12/08/2015 10:50, Savolainen, Petri (Nokia - FI/Espoo) > wrote: > > Bill, > > hash_tbl_entry is now uintptr_t, which is 64 bits on a > 64 bit build and > > 32 bits on a 32 bit build. These shifts and all other > lines that expect > that hash_tbl_entry (pointer size) is 64 bits should be > updated also. > > -Petri > > Yes, that is what I said before. For now we can go with > simple using 64 > bit hash entries for 32 bit case also. > Where 6 upper bits will be for defining first or secondary > hash and it's > size. And all lower 32 for address pointer. > I.e. 26 bits will be lost. > > #define PRIMARY_HASH_TBL_SIZE (16 * 1024) > 26 * PRIMARY_HASH_TBL_SIZE = 425984 = 53248 bytes = 13 pages; > > #define SECONDARY_HASH_TBL_SIZE 128 > 26 * SECONDARY_HASH_TBL_SIZE = 3328 = 416 bytes > > So we lost for 32 bits some memory but code has to work in > the same way > as for 64 bits. > After that if memory efficient fix will be needed for > somebody than he > can fix it. > Or we will reuse some odp hash table api instead of > odp_name_table.c. > > So for now I would do quick fix with some comment that > there is unused > memory for 32 bit case. > > --- a/platform/linux-generic/odp_name_table.c > +++ b/platform/linux-generic/odp_name_table.c > @@ -93,7 +93,7 @@ typedef struct { > * NOT zero then this values points to a (linked list of) > name_table_entry_t > * records AND the bottom 6 bits are the length of this > list. > */ > -typedef uintptr_t hash_tbl_entry_t; > +typedef uint64_t hash_tbl_entry_t; > > > BR, > Maxim. > > > odp_name_table.c: In function 'make_hash_tbl_entry': > odp_name_table.c:570:20: error: cast from pointer to integer > of different size [-Werror=pointer-to-int-cast] > hash_tbl_entry = (hash_tbl_entry_t)name_tbl_entry; > > > static hash_tbl_entry_t make_hash_tbl_entry(name_tbl_entry_t > *name_tbl_entry, > uint32_t > entry_cnt) > { > hash_tbl_entry_t hash_tbl_entry; > uint32_t new_entry_cnt; > > new_entry_cnt = MIN(entry_cnt + 1, 0x3F); > hash_tbl_entry = > (hash_tbl_entry_t)(uintptr_t)name_tbl_entry; > hash_tbl_entry &= ~0x3F; > hash_tbl_entry |= new_entry_cnt; > return hash_tbl_entry; > } > > > And compiler is still happy with the casts? Maybe the casting > issue was actually caused by removal of the uintptr_t cast above. > > -Petri > > Petri, Looks like you did not apply that patch. If you apply it > and do change to uint64_t everything compiles. > > Maxim. > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
--- a/platform/linux-generic/odp_name_table.c +++ b/platform/linux-generic/odp_name_table.c @@ -93,7 +93,7 @@ typedef struct { * NOT zero then this values points to a (linked list of) name_table_entry_t * records AND the bottom 6 bits are the length of this list. */ -typedef uintptr_t hash_tbl_entry_t; +typedef uint64_t hash_tbl_entry_t;