On Tue, Apr 4, 2017 at 3:40 PM, Brian Brooks <brian.bro...@arm.com> wrote:
> On 04/04 23:02:15, Maxim Uvarov wrote:
>> On 04/04/17 22:48, Brian Brooks wrote:
>> > On 04/04 21:57:29, Maxim Uvarov wrote:
>> >> On 04/04/17 21:47, Brian Brooks wrote:
>> >>> Signed-off-by: Ola Liljedahl <ola.liljed...@arm.com>
>> >>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
>> >>> Reviewed-by: Brian Brooks <brian.bro...@arm.com>
>> >>> ---
>> >>> platform/linux-generic/include/odp_config_internal.h | 7 +++++++
>> >>> 1 file changed, 7 insertions(+)
>> >>>
>> >>> diff --git a/platform/linux-generic/include/odp_config_internal.h
>> >>> b/platform/linux-generic/include/odp_config_internal.h
>> >>> index dadd59e7..4822e69d 100644
>> >>> --- a/platform/linux-generic/include/odp_config_internal.h
>> >>> +++ b/platform/linux-generic/include/odp_config_internal.h
>> >>> @@ -22,6 +22,13 @@ extern "C" {
>> >>> #define ODP_CONFIG_QUEUES 1024
>> >>>
>> >>> /*
>> >>> + * Maximum queue depth. Maximum number of elements that can be stored
>> >>> in a
>> >>> + * queue. This value is used only when the size is not explicitly
>> >>> provided
>> >>> + * during queue creation.
>> >>> + */
>> >>> +#define ODP_CONFIG_QUEUE_SIZE 4096
>> >>> +
>> >>
>> >> internal things should not start this ODP_ . Just follow syntax in that
>> >> file.
>> >
>> > Please take another look at the file, e.g. ODP_CONFIG_QUEUES,
>> > ODP_CONFIG_POOLS..
>> >
>>
>> yes, some names has ODP_. That is bad and confusing. But we did it at
>> the beginning and did not clean up later. But for new code it's better
>> to not overlap with api prefixes.
>
> OK. We can address it in a follow-up patch, otherwise it will still
> remain 'bad and confusing' if we only address it in this patch.
>
>> >> Patch has to be merge to patch where it is used.
>> >
>> > Can you please explain?
>> >
>>
>> This patch adds new define. But it's not used in code of this patch.
>> That is very confusing for reading.
>
> This is also another pre-existing condition.
I see no problem with adding the #define in one patch and using it in
the next. That's no different than having a new API defined in one
patch and then implemented in another. The real issue here is that
this patch is unnecessarily small. A single part can cover multiple
files, so in this case having this in the same part that makes use of
it would keep the number of parts down. In general you don't want to
have patches that consist of a large number of parts. I think we
picked 15 as the preferred upper limit. Consolidating trivial changes
into a single part helps keep that number in check.
>
>> So this patch should be combined
>> with patch which uses this define. So people can see why you moved this
>> to config and how do you use it.
>>
>> Maxim.
>>
>>
>>
>>
>> >> Maxim.
>> >>
>> >>> +/*
>> >>> * Maximum number of ordered locks per queue
>> >>> */
>> >>> #define CONFIG_QUEUE_MAX_ORD_LOCKS 4
>> >>>
>> >>
>>
