Hello, On Mon, 23 Feb 2015, Alex Gartrell wrote: > > Can a new create_dest(svc, bool for_update) method > > solve such problem? Such modifications are serialized, so > > create_dest can attach new structure to sched_data just before > > the add_dest/upd_dest call. IPVS does not clone dests on > > update but scheduler may need this for RCU update. > > create_dest should be called early in __ip_vs_update_dest. > > Note that kfree(dest) is not enough, see ip_vs_dest_free(), > > more fields may be allocated before __ip_vs_update_dest is > > reached. > > More context on the custom scheduler: > > It's a consistent hash ring algorithm which requires sorting all of the > members and then inserting them one at a time (a entry per weight). It > requires a massive allocation to load balance to the number of machines we > need to. The schedulers do not need RCU for list traversal if they implement their own lists. But if dest is returned from the schedule() method this dest is not freed while in RCU grace period. And this logic is not part of the scheduler. So, the scheduler may use some lock(s) which is bad for SMP (most schedulers already use such lock) and the only thing that they should care if their schedule method uses RCU for their internal structures. Lock is needed only as syncronization between schedule() and the other methods that maintain internal structures. > Right here I'm going to admit that this is not ideal. This is somewhat > "inherited," but I'm currently stuck solving this problem so I'll make the > best of it :) > > So now we're in this position where adds, deletes, and updates all can force > us to recalculate this giant hash ring. We can either simply lock the ring > (but this actually takes long enough with a large number of real servers with > large weights that it causes soft lock up detection!) or do RCU (a new > allocation every time). We're trying out the RCU thing now which is where > this hurt us. > > One aside, if it's safe to call synchronize_rcu in that path, then it may > solve our most painful allocation problem in part. Yes, it is safe to use synchronize_rcu in scheduler, except in schedule() method, of course. IPVS holds just a mutex on configuration changes. But synchronize_rcu should be avoided, it is always possible to use another way. > > Patch 4: > > As for del_dest result, failure should not happen, > > we should be able to flush dests on module cleanup. I think, there > > should not be any allocations in this case. > > As mentioned above, we may need to reallocate in this path. But it does > offend my sensibilities that this is the case, so I think it'd be fair to > change the del_dest function to return void and we'd just find another way to > do it. Looks like, patch 4 can be used, just use 'dest->flags = old_flags;' before list_add_rcu. Because the module removal will not call del_dest, the done_service method called by ip_vs_unbind_scheduler() is called when service is deleted. More details here: - init_service: - can be called when service is edited, eg. when changing scheduler - we can see empty list or the list of all destinations before the scheduler is changed - done_service: - sched_data must be freed after grace period and module exit should be delayed with synchronize_rcu to wait all RCU read-side critical sections to complete - can be called without calling del_dest when service is deleted - add_dest: - after dest is linked - del_dest: - after dest is unlinked - not called if service is deleted, in this case only done_service is called - upd_dest: - after dest is edited (new weight) - schedule: - called under RCU read lock, services are in list protected by RCU - needs _rcu if using svc->destinations - can call ip_vs_dest_hold - scheduler can hold dests in its state long after they are unlinked from svc, even without providing del_dest handler. - should check for IP_VS_DEST_F_AVAILABLE if using stale dest from sched_data structures - should use _bh suffix for sched_lock or other locks Regards -- Julian Anastasov <ja@xxxxxx> -- To unsubscribe from this list: send the line "unsubscribe lvs-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
