Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use process_one_line() instead of fscanf() and remove redundancy check in add_one_irq_to_db() #142

Merged
merged 2 commits into from Nov 12, 2019

Conversation

@yeyunfeng-dev
Copy link
Contributor

yeyunfeng-dev commented Nov 11, 2019

irqbalance: use process_one_line() instead of fscanf()
irqbalance: remove redundancy check in add_one_irq_to_db()

The logic using fscanf() to get data from open file can be instead by
process_one_line(), and provide two functions get_hex() and get_int()
to convert string to integer. also use get_int() instead of
get_offline_status() and get_packageid().

Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
Before calling add_one_irq_to_db(), it has been checked by calling
get_irq_info(), so remove unnecessary redundancy check.

Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
@yeyunfeng-dev yeyunfeng-dev changed the title irqbalance: use process_one_line() instead of fscanf() use process_one_line() instead of fscanf() and remove redundancy check in add_one_irq_to_db() Nov 11, 2019
@nhorman

This comment has been minimized.

Copy link
Contributor

nhorman commented Nov 12, 2019

This doesn't seem right. You're claiming that get_irq_info is a valid substitue for checking to see if the irq is on the banned list, but its not. If an irq has been added to the banned list, it won't ever be added to the irq db, and with this change, it will

@yeyunfeng-dev

This comment has been minimized.

Copy link
Contributor Author

yeyunfeng-dev commented Nov 12, 2019

get_irq_info() find irq_info from interrupts_db and banned_irqs, the logic is same as the check logic in add_one_irq_to_db(),isn't it ? thanks.

@nhorman

This comment has been minimized.

Copy link
Contributor

nhorman commented Nov 12, 2019

No, get_irq_info returns the irq_info struct for an interrupt, be it on the banned_list or the actual irq db list. is_banned_irq tells you if the interrupt is on the banned list or not

@yeyunfeng-dev

This comment has been minimized.

Copy link
Contributor Author

yeyunfeng-dev commented Nov 12, 2019

I agree with you, but the check logic in add_one_irq_to_db() not only call is_banned_irq(), but also check is irq in interrupts_db list:
/*
* First check to make sure this isn't a duplicate entry
*/
entry = g_list_find_custom(interrupts_db, hint, compare_ints);
if (entry) {
log(TO_CONSOLE, LOG_INFO, "DROPPING DUPLICATE ENTRY FOR IRQ %d on path %s\n", irq, devpath);
return NULL;
}

@nhorman

This comment has been minimized.

Copy link
Contributor

nhorman commented Nov 12, 2019

yes, which your commit removes. See the add_new_irq path. I think you're assuming that all callers of add_one_irq_to_db check the banned list, and thats not the case

@yeyunfeng-dev

This comment has been minimized.

Copy link
Contributor Author

yeyunfeng-dev commented Nov 12, 2019

`
static void add_new_irq(int irq, struct irq_info *hint, GList *proc_interrupts)
{
struct irq_info *new;
struct user_irq_policy pol;

new = get_irq_info(irq);   //--> check irq whether is in interrupts_db  banned_irqs list or not
if (new)
	return;

/* Set NULL devpath for the irq has no sysfs entries */
get_irq_user_policy(NULL, irq, &pol);
if ((pol.ban == 1) || check_for_irq_ban(irq, proc_interrupts)) { /*FIXME*/
	add_banned_irq(irq, &banned_irqs);   //--> only add to the banned_irqs list
	new = get_irq_info(irq);
} else
	new = add_one_irq_to_db(NULL, hint, &pol); //-->so it is unnecessary check again

`
look at the code add_new_irq(), I have make some comments, before invoking add_one_irq_to_db(), it has already been checked by get_irq_info(). Am I right? thanks.

@nhorman

This comment has been minimized.

Copy link
Contributor

nhorman commented Nov 12, 2019

you're still missing my point. In add_new_irq, yes, you check to see if you have already added this irq, which checks both the irq_db and banned lists, thats fine. but in the event you don't find an irq, you check your policy, and potentially add the irq to the banned list, then call add_one_irq_to_db. In add_one_irq_to_db, you have removed the check for is_banned_irq, and so, in that situation you will wind up adding an irq that you have already put on the banned list to the irq db. Dont do that.

@yeyunfeng-dev

This comment has been minimized.

Copy link
Contributor Author

yeyunfeng-dev commented Nov 12, 2019

the point I can't understand is that:
" you check your policy, and potentially add the irq to the banned list, then call add_one_irq_to_db"

if the irq add to banned list, it will not call add_one_irq_to_db(), it is two different path code:

if ((pol.ban == 1) || check_for_irq_ban(irq, proc_interrupts)) { /*FIXME*/ add_banned_irq(irq, &banned_irqs); new = get_irq_info(irq); } else new = add_one_irq_to_db(NULL, hint, &pol);

thanks.

@nhorman

This comment has been minimized.

Copy link
Contributor

nhorman commented Nov 12, 2019

Oh, my bad, you're right, I missed the else clause there.

@nhorman nhorman merged commit f1fd0b5 into Irqbalance:master Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.