> On 4 Nov 2014, at 09:50, Alexander Bluhm <alexander.bl...@gmx.net> wrote:
>
> On Thu, Oct 30, 2014 at 09:55:35PM -0400, Ted Unangst wrote:
>> On Thu, Oct 30, 2014 at 22:10, Alexander Bluhm wrote:
>>> +
>>> + /* Avoid user land starvation. */
>>> + yield();
>>
>> I think this is the responsibility of the taskq thread, not the
>> individual task.
>
> I am not sure about this. Without an explicit yield() I see user
> land starvation, but I only used a virtual test machine. Note that
> the net_livelocks detection does not work as softtimer may interrupt
> the kernel thread. Originally I have put the yield() there as copy
> through user land always calls the scheduler. I have removed it
> for now from this diff. It has no impact on throughput.
i agree with tedu@. blambert@ had a diff to make task threads yield when they
got too busy so the tasks themselves wouldnt have to worry about that.
> I have merged the diff with sosplice pool. Now the struct socket
> size remains at 392 bytes, but struct sosplice grows from 88 to 136
> bytes. To save memory the thread is only created on demand when
> the machine uses splicing for the first time.
>
> ok?
have you considered using softintr_establish(IPL_SOFTNET, ...) instead of tasks?
either way, i am wary of moving work to other contexts where they could
possibly overlap with work on different cpus. eg, what happens if you're in the
middle of running sotask on one cpu while another run sounsplice? on the other
hand, that isnt possible at the moment so maybe i shouldnt care right now.
>
> bluhm
>
> Index: kern/uipc_socket.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.134
> diff -u -p -r1.134 uipc_socket.c
> --- kern/uipc_socket.c 3 Nov 2014 17:20:46 -0000 1.134
> +++ kern/uipc_socket.c 3 Nov 2014 23:03:32 -0000
> @@ -56,6 +56,7 @@ void sbsync(struct sockbuf *, struct mbu
> int sosplice(struct socket *, int, off_t, struct timeval *);
> void sounsplice(struct socket *, struct socket *, int);
> void soidle(void *);
> +void sotask(void *, void *);
> int somove(struct socket *, int);
>
> void filt_sordetach(struct knote *kn);
> @@ -82,6 +83,7 @@ int sominconn = SOMINCONN;
> struct pool socket_pool;
> #ifdef SOCKET_SPLICE
> struct pool sosplice_pool;
> +struct taskq *sosplice_taskq;
> #endif
>
> void
> @@ -1038,6 +1040,7 @@ sorflush(struct socket *so)
> #define so_splicemax so_sp->ssp_max
> #define so_idletv so_sp->ssp_idletv
> #define so_idleto so_sp->ssp_idleto
> +#define so_splicetask so_sp->ssp_task
>
> int
> sosplice(struct socket *so, int fd, off_t max, struct timeval *tv)
> @@ -1046,6 +1049,9 @@ sosplice(struct socket *so, int fd, off_
> struct socket *sosp;
> int s, error = 0;
>
> + if (sosplice_taskq == NULL)
> + sosplice_taskq = taskq_create("sosplice", 1, IPL_SOFTNET);
> +
> if ((so->so_proto->pr_flags & PR_SPLICE) == 0)
> return (EPROTONOSUPPORT);
> if (so->so_options & SO_ACCEPTCONN)
> @@ -1123,6 +1129,7 @@ sosplice(struct socket *so, int fd, off_
> else
> timerclear(&so->so_idletv);
> timeout_set(&so->so_idleto, soidle, so);
> + task_set(&so->so_splicetask, sotask, so, NULL);
>
> /*
> * To prevent softnet interrupt from calling somove() while
> @@ -1146,6 +1153,7 @@ sounsplice(struct socket *so, struct soc
> {
> splsoftassert(IPL_SOFTNET);
>
> + task_del(sosplice_taskq, &so->so_splicetask);
> timeout_del(&so->so_idleto);
> sosp->so_snd.sb_flagsintr &= ~SB_SPLICE;
> so->so_rcv.sb_flagsintr &= ~SB_SPLICE;
> @@ -1168,6 +1176,24 @@ soidle(void *arg)
> splx(s);
> }
>
> +void
> +sotask(void *arg1, void *arg2)
> +{
> + struct socket *so = arg1;
> + int s;
> +
> + s = splsoftnet();
> + if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
> + /*
> + * We may not sleep here as sofree() and unsplice() may be
> + * called from softnet interrupt context. This would remove
> + * the socket during somove().
> + */
> + somove(so, M_DONTWAIT);
> + }
> + splx(s);
> +}
> +
> /*
> * Move data from receive buffer of spliced source socket to send
> * buffer of drain socket. Try to move as much as possible in one
> @@ -1435,6 +1461,7 @@ somove(struct socket *so, int wait)
> #undef so_splicemax
> #undef so_idletv
> #undef so_idleto
> +#undef so_splicetask
>
> #endif /* SOCKET_SPLICE */
>
> @@ -1442,8 +1469,20 @@ void
> sorwakeup(struct socket *so)
> {
> #ifdef SOCKET_SPLICE
> - if (so->so_rcv.sb_flagsintr & SB_SPLICE)
> - (void) somove(so, M_DONTWAIT);
> + if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
> + /*
> + * TCP has a sendbuffer that can handle multiple packets
> + * at once. So queue the stream a bit to accumulate data.
> + * The sosplice thread will call somove() later and send
> + * the packets calling tcp_output() only once.
> + * In the UDP case, send out the packets immediately.
> + * Using a thread would make things slower.
> + */
> + if (so->so_proto->pr_flags & PR_WANTRCVD)
> + task_add(sosplice_taskq, &so->so_sp->ssp_task);
> + else
> + somove(so, M_DONTWAIT);
> + }
> if (isspliced(so))
> return;
> #endif
> @@ -1457,7 +1496,8 @@ sowwakeup(struct socket *so)
> {
> #ifdef SOCKET_SPLICE
> if (so->so_snd.sb_flagsintr & SB_SPLICE)
> - (void) somove(so->so_sp->ssp_soback, M_DONTWAIT);
> + task_add(sosplice_taskq,
> + &so->so_sp->ssp_soback->so_sp->ssp_task);
> #endif
> sowakeup(so, &so->so_snd);
> }
> Index: sys/socketvar.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/sys/socketvar.h,v
> retrieving revision 1.57
> diff -u -p -r1.57 socketvar.h
> --- sys/socketvar.h 3 Nov 2014 17:20:46 -0000 1.57
> +++ sys/socketvar.h 3 Nov 2014 21:39:39 -0000
> @@ -34,6 +34,7 @@
>
> #include <sys/selinfo.h> /* for struct selinfo */
> #include <sys/queue.h>
> +#include <sys/task.h>
> #include <sys/timeout.h>
>
> #ifndef _SOCKLEN_T_DEFINED_
> @@ -91,6 +92,7 @@ struct socket {
> off_t ssp_max; /* maximum number of bytes */
> struct timeval ssp_idletv; /* idle timeout */
> struct timeout ssp_idleto;
> + struct task ssp_task; /* task for somove */
> } *so_sp;
> /*
> * Variables for socket buffering.
