|
On Sat, Aug 27, 2022 at 08:41:50PM +0300, Julian Anastasov wrote:
> This patchset implements stats estimation in
> kthread context. Simple tests do not show any problem.
> Please review, comment, test, etc.
Thank you for this, Julian.
> RCU Locking:
>
> - when RCU preemption is enabled the kthreads use just RCU
> lock for walking the chains and we do not need to reschedule.
> May be this is the common case for distribution kernels.
> In this case ip_vs_stop_estimator() is completely lockless.
Yes, it is the case for SUSE since SLES 15.4, which is a recent release.
> - when RCU preemption is not enabled, we reschedule by using
> refcnt for every estimator to track if the currently removed
> estimator is used at the same time by kthread for estimation.
> As RCU lock is unlocked during rescheduling, the deletion
> should wait kd->mutex, so that a new RCU lock is applied
> before the estimator is freed with RCU callback.
I believe allowing the kthread the possibility to block in each iteration - for
each estimator - introduces some problems:
1. Non-preemptive kernels should be optimized for throughput not for latency
Using the figure reported earlier (50,000 services required 200 ms of
processing time) it takes roughly 3 ms to process one chain (32 * 1024 / 50
services). The processing time per chain will vary with the number of NUMA
nodes and CPUs. Nevertheless, this number comparable with the processing time
limit of __do_softirq() - 2 ms, which gets converted to 1 jiffy. In term of
latency of non-preemptive kernels, it is entirely resonable to let one chain be
processed without rescheduling the kthread.
2. The priority of the kthreads could be set to lower values than the default
priority for SCHED_OTHER. If a user space process was trying to stop an
estimator the pointer to which is held by a currently sleeping kthread this
would constitute priority inversion. The kd->mutex would not be released until
the lower priority thread, the kthread, has started running again. AFAIK, the
mutex() locking primitive does not implement priority inheritance on
non-preemptive kernels.
3. Blocking while in ip_vs_estimation_chain() will results in wrong estimates
for the remaining estimators in a chain. The 2 second interval would not be
kept, rate estimates would be overestimated in that interval and underestimated
in one of the future intervals.
In my opinion, any of the above reasons is sufficient to remove
ip_vs_est_cond_resched_rcu(), ip_vs_est_wait_resched() and kd->mutex.
> - As stats are now RCU-locked, tot_stats, svc and dest which
> hold estimator structures are now always freed from RCU
> callback. This ensures RCU grace period after the
> ip_vs_stop_estimator() call.
I think this is sound.
> Kthread data:
>
> - every kthread works over its own data structure and all
> such structures are attached to array
It seems to me there is no upper bound on the number of kthreads that could be
forked. Therefore, it should be possible to devise an attack that would force
the system to run out of PIDs:
1. Start adding services so that all chains of kthread A would be used.
2. Add one more service to force the forking of kthread B, thus advancing
ipvs->est_add_ktid.
3. Remove all but one service from kthread A.
4. Repeat steps 1-3 but with kthread B.
I think I could come up with a reproducer if need be.
> - to add new estimators we use the last added kthread
> context (est_add_ktid). The new estimators are linked to
> the chain just before the estimated one, based on add_row.
> This ensures their estimation will start after 2 seconds.
> If estimators are added in bursts, common case if all
> services and dests are initially configured, we may
> spread the estimators to more chains. This will reduce
> the chain imbalance.
>
> - the chain imbalance is not so fatal when we use
> kthreads. We design each kthread for part of the
> possible CPU usage, so even if some chain exceeds its
> time slot it would happen all the time or sporadic
> depending on the scheduling but still keeping the
> 2-second interval. The cpulist isolation can make
> the things more stable as a 2-second time interval
> per estimator.
Unbalanced chains would not be fatal to the system but there is risk of
introducing scheduling latencies tens or even hundreds of milliseconds long.
There are patterns of adding and removing chains that would results in chain
imbalance getting so severe that a handful of chains would have estimators in
them while the rest would be empty or almost empty. Some examples:
1. Adding a new service each second after sleeping for 1 second. This would use
the add_row value at the time of adding the estimator, which would result in 2
chains holding all the estimators.
2. Repeated addition and removal of services. There would always be more
services added than removed. The additions would be carried out in bursts. The
forking of a new kthread would not be triggered because the number of services
would stay under IPVS_EST_MAX_COUNT (32 * 1024).
The problem is that the code does not guarantee that the length of chains
always stays under IPVS_EST_MAX_COUNT / IPVS_EST_NCHAINS (32 * 1024 / 50)
estimators. A check and a cycle iterating over available rows could be added to
ip_vs_start_estimator() to find the rows that still have fewer estimators than
IPVS_EST_MAX_COUNT / IPVS_EST_NCHAINS. This would come at the expense of having
inaccurate estimates for a few intervals but I think the trade-off is worth it.
Also, the estimates will be inaccurate when estimators are added in bursts. If,
depending on how services are added and removed, the code could introduce
scheduling latencies there would be someone running into this sooner or later.
The probability of severe chain imbalance being low is not good enough there
should be a guarantee.
--
Jiri Wiesner
SUSE Labs
|
