Hi Vincent,
On 06/28/2017 08:45 PM, Vincent Guittot wrote:
> Hi Georgi,
>
> On 27 June 2017 at 19:49, Georgi Djakov <georgi.djakov@xxxxxxxxxx> wrote:
>
> [snip]
>
>> +
>> +static int interconnect_aggregate(struct interconnect_node *node,
>> + struct interconnect_creq *creq)
>> +{
>> + int ret = 0;
>> +
>> + mutex_lock(&node->icp->lock);
>> +
>> + if (node->icp->ops->aggregate) {
>> + ret = node->icp->ops->aggregate(node, creq);
>> + if (ret) {
>> + pr_info("%s: error (%d)\n", __func__, ret);
>> + goto out;
>> + }
>> + } else {
>> + /* do not aggregate by default */
>> + struct icp *icp = node->icp;
>> +
>> + icp->creq.avg_bw = creq->avg_bw;
>> + icp->creq.peak_bw = creq->peak_bw;
>
> Does it means that by default the last caller defines the bandwidth
> for everybody ?
> IMHO, having a default aggregation policy that sums the avg_bw of all
> request of the node
> and that gets the max of peak_bw of all request of a node is better
>
Yes, i had this in one of the previous versions, but then i removed the
aggregation by default. Will put it back. Thanks!
>> + }
>> +
>> +out:
>> + mutex_unlock(&node->icp->lock);
>> + return ret;
>> +}
>> +
>> +/**
>> + * interconnect_set() - set constraints on a path between two endpoints
>> + * @path: reference to the path returned by interconnect_get()
>> + * @creq: request from the consumer, containing its requirements
>> + *
>> + * This function is used by an interconnect consumer to express its own needs
>> + * in term of bandwidth and QoS for a previously requested path between two
>> + * endpoints. The requests are aggregated and each node is updated accordingly.
>> + *
>> + * Returns 0 on success, or an approproate error code otherwise.
>> + */
>> +int interconnect_set(struct interconnect_path *path,
>> + struct interconnect_creq *creq)
>> +{
>> + struct interconnect_node *next, *prev = NULL;
>> + size_t i;
>> + int ret = 0;
>> +
>> + for (i = 0; i < path->num_nodes; i++, prev = next) {
>> + next = path->reqs[i].node;
>> +
>> + if (!next || !prev)
>
> This needs a comment with an explanation about why you don't do
> anything in this case
Ok.
>
>> + continue;
>> +
>> + if (next->icp != prev->icp)
>
> This needs a comment with an explanation about why you don't do
> anything in this case
Ok.
>
>> + continue;
>> +
>> + /* aggregate requests from consumers */
>
> you should update the path->reqs[i].avg_bw and path->reqs[i].peak_bw
> with creq values
> before aggregating the requests from the different consumer of a node ?
>
> path->reqs[i].avg_bw = creq->avg_bw
> path->reqs[i].peak_bw = creq->peak_bw
I am updating them currently in the vendor implementation of the
aggregate() function, but this was probably not the right place,
so i will move it here instead. Thanks for the comments!
BR,
Georgi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
