Re: [PATCH V4 2/9] interconnect: Set peak requirement as twice of average

[Georgi Djakov <georgi.djakov@xxxxxxxxxx>]

Hi Akash,

On 4/28/20 12:46, Akash Asthana wrote:
> Hi Georgi,
> 
> On 4/23/2020 3:01 PM, Georgi Djakov wrote:
>> Hi Akash,
>>
>> On 4/15/20 13:23, Akash Asthana wrote:
>>> Lot of ICC clients are not aware of their actual peak requirement,
>>> most commonly they tend to guess their peak requirement as
>>> (some factor) * avg_bw.
>>>
>>> Centralize random peak guess as twice of average, out into the core
>>> to maintain consistency across the clients. Client can always
>>> override this setting if they got a better idea.
>> I am still not convinced that this is a good idea. If the factor is a random
>> value, then i think that the default factor should be 1.
>>
>> According to your previous reply, it seems that from geni we are requesting
>> double peak bandwidth to compensate for other clients which are not requesting
>> bandwidth for themselves. IMO, this is a bit hacky.
>>
>> Instead of requesting double peak bandwidth, IIUC the correct thing to do here
>> is to request peak_bw = avg_bw for geni. And instead of trying to compensate for
>> other clients "stealing" bandwidth, can't we make these clients vote for their
>> own bandwidth? Or if they really can't, this should be handled elsewhere - maybe
>> in the interconnect platform driver we can reserve some amount of minimum
>> bandwidth for such cases?
> 
> Okay, probably we can correct clients vote for their own bandwidth or reserve
> some minimum BW from interconnect platform driver is case of any latency issue
> observed.

Yes, this sounds like the correct thing to do.

> 
> I will drop this change in next version.
> 
> Will it create any difference if  peak_bw = 0 instead of peak_bw = avg_bw? In my
> understanding peak_bw <= avg_bw is no-ops, it won't impact the NOC speed.

It will not have impact on the NOC speed, but it does not make much logical
sense to have peak_bw = 0 or peak_bw < avg_bw. In the geni case, i think what
we want to do is peak_bw = avg_bw.

Thanks,
Georgi