lkml.org 
[lkml]   [2000]   [Aug]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH] thread wakeup fix for 2.4.0-test7
    Date
    In message <200008281632.UAA04197@ms2.inr.ac.ru> you write:
    > > > Also, lingering (i.e. waiting for looong time) in fput() smells
    > > > interesting. 8)
    > >
    > > Yes? Receiving process doesn't care to do recvmsg() on SCM_RIGHTS
    > > datagram. What should happen on close(), again?
    >
    > Nothing. close() decreases f_users and exits, because f_users is not zero.

    I didn't use second reference count because it needs to be associated
    with each fd in struct files_struct. NOT like f_count, which is in
    `struct file'.

    Consider:

    fd2 = dup(fd1);
    pthread_create(thread1);
    pthread_create(thread2);
    fork() => child1;


    Thread 0 Thread 1 Thread 2 Child 1

    read(fd1); read(fd2); read(fd1);

    close(fd1);
    MUST -EBADF MUST NOT -EBADF MUST NOT -EBADF

    And then it becomes obvious that count is not needed, it is always 1
    or 0; ie. only threads waiting on same fd in same struct files_struct
    must be woken. My patch (at head of thread) was cc:'d to
    vger.rutgers.edu by mistake. See below for original post.

    Hope this clarifies,
    Rusty.
    --
    Hacking time.

    ================
    Hi all,

    BUG: thread 1 is doing poll() on an fd, thread 2 closes it,
    and thread 1 doesn't wake up. Included is a small test program and
    Makefile.

    Simply waking up everyone polling on a filp on every close
    would suck for performance, so I only wake up those waiting with the
    same struct files_struct (ie. those sharing the file table).
    Unfortunately, the wait queue is not in the common part of the inode,
    so this needs to be implemented for every pollable device. I have
    only implemented it for sockets, pipes and fifos.

    Before patch:
    rusty@unpatched$ sudo make
    UDP test
    unwoken child: SUCCESS
    FAILED: didn't wake me
    unwoken: SUCCESS
    TCP test
    unwoken child: SUCCESS
    FAILED: didn't wake me
    unwoken: SUCCESS
    RAW test
    unwoken child: SUCCESS
    FAILED: didn't wake me
    unwoken: SUCCESS
    PIPE test
    unwoken child: SUCCESS
    FAILED: didn't wake me
    unwoken: SUCCESS
    FIFO test
    unwoken child: SUCCESS
    FAILED: didn't wake me
    unwoken: SUCCESS
    CHARDEV test
    unwoken child: SUCCESS
    FAILED: didn't wake me
    unwoken: SUCCESS

    After patch:
    rusty@patched $ sudo make
    cc -Wall -W -o polltest polltest.c -lpthread
    UDP test
    woken: SUCCESS
    unwoken: SUCCESS
    unwoken child: SUCCESS
    TCP test
    woken: SUCCESS
    unwoken: SUCCESS
    unwoken child: SUCCESS
    RAW test
    woken: SUCCESS
    unwoken: SUCCESS
    unwoken child: SUCCESS
    PIPE test
    woken: SUCCESS
    unwoken: SUCCESS
    unwoken child: SUCCESS
    FIFO test
    woken: SUCCESS
    unwoken: SUCCESS
    unwoken child: SUCCESS
    CHARDEV test
    unwoken: SUCCESS
    unwoken child: SUCCESS
    FAILED: didn't wake me

    Cheers,
    Rusty.
    --
    Hacking time.

    diff -ur -X /tmp/filexiytUc --minimal linux-2.4.0-test7-official/fs/open.c working-2.4.0-test7/fs/open.c
    --- linux-2.4.0-test7-official/fs/open.c Fri Aug 25 13:11:36 2000
    +++ working-2.4.0-test7/fs/open.c Fri Aug 25 16:18:17 2000
    @@ -791,6 +791,8 @@
    unlock_kernel();
    }
    locks_remove_posix(filp, id);
    + if (filp->f_op && filp->f_op->fdclosed)
    + filp->f_op->fdclosed(filp, id);
    fput(filp);
    return retval;
    }
    diff -ur -X /tmp/filexiytUc --minimal linux-2.4.0-test7-official/fs/pipe.c working-2.4.0-test7/fs/pipe.c
    --- linux-2.4.0-test7-official/fs/pipe.c Fri Aug 25 13:11:36 2000
    +++ working-2.4.0-test7/fs/pipe.c Fri Aug 25 15:39:50 2000
    @@ -375,6 +375,26 @@
    return 0;
    }

    +static void pipe_fdclosed(struct file *filp, struct files_struct *files)
    +{
    + struct inode *inode = filp->f_dentry->d_inode;
    + struct list_head *i;
    + unsigned long flags;
    +
    + /* Wake up everyone using same file table. */
    + down(PIPE_SEM(*inode));
    + wq_write_lock_irqsave(&PIPE_WAIT(*inode)->lock, flags);
    + for (i = PIPE_WAIT(*inode)->task_list.next;
    + i != &PIPE_WAIT(*inode)->task_list;
    + i = i->next) {
    + wait_queue_t *curr = list_entry(i, wait_queue_t, task_list);
    + if (curr->task->files == files)
    + wake_up_process(curr->task);
    + }
    + wq_write_unlock_irqrestore(&PIPE_WAIT(*inode)->lock, flags);
    + up(PIPE_SEM(*inode));
    +}
    +
    /*
    * The file_operations structs are not static because they
    * are also used in linux/fs/fifo.c to do operations on FIFOs.
    @@ -387,6 +407,7 @@
    ioctl: pipe_ioctl,
    open: pipe_read_open,
    release: pipe_read_release,
    + fdclosed: pipe_fdclosed,
    };

    struct file_operations write_fifo_fops = {
    @@ -397,6 +418,7 @@
    ioctl: pipe_ioctl,
    open: pipe_write_open,
    release: pipe_write_release,
    + fdclosed: pipe_fdclosed,
    };

    struct file_operations rdwr_fifo_fops = {
    @@ -407,6 +429,7 @@
    ioctl: pipe_ioctl,
    open: pipe_rdwr_open,
    release: pipe_rdwr_release,
    + fdclosed: pipe_fdclosed,
    };

    struct file_operations read_pipe_fops = {
    @@ -417,6 +440,7 @@
    ioctl: pipe_ioctl,
    open: pipe_read_open,
    release: pipe_read_release,
    + fdclosed: pipe_fdclosed,
    };

    struct file_operations write_pipe_fops = {
    @@ -427,6 +451,7 @@
    ioctl: pipe_ioctl,
    open: pipe_write_open,
    release: pipe_write_release,
    + fdclosed: pipe_fdclosed,
    };

    struct file_operations rdwr_pipe_fops = {
    @@ -437,6 +462,7 @@
    ioctl: pipe_ioctl,
    open: pipe_rdwr_open,
    release: pipe_rdwr_release,
    + fdclosed: pipe_fdclosed,
    };

    struct inode* pipe_new(struct inode* inode)
    diff -ur -X /tmp/filexiytUc --minimal linux-2.4.0-test7-official/include/linux/fs.h working-2.4.0-test7/include/linux/fs.h
    --- linux-2.4.0-test7-official/include/linux/fs.h Fri Aug 25 13:11:43 2000
    +++ working-2.4.0-test7/include/linux/fs.h Fri Aug 25 13:38:43 2000
    @@ -746,6 +746,7 @@
    int (*lock) (struct file *, int, struct file_lock *);
    ssize_t (*readv) (struct file *, const struct iovec *, unsigned long, loff_t *);
    ssize_t (*writev) (struct file *, const struct iovec *, unsigned long, loff_t *);
    + void (*fdclosed) (struct file *, struct files_struct *);
    };

    struct inode_operations {
    diff -ur -X /tmp/filexiytUc --minimal linux-2.4.0-test7-official/include/linux/net.h working-2.4.0-test7/include/linux/net.h
    --- linux-2.4.0-test7-official/include/linux/net.h Fri Aug 25 12:10:21 2000
    +++ working-2.4.0-test7/include/linux/net.h Fri Aug 25 13:38:43 2000
    @@ -82,6 +82,7 @@

    struct scm_cookie;
    struct vm_area_struct;
    +struct files_struct;

    struct proto_ops {
    int family;
    @@ -108,6 +109,7 @@
    int (*sendmsg) (struct socket *sock, struct msghdr *m, int total_len, struct scm_cookie *scm);
    int (*recvmsg) (struct socket *sock, struct msghdr *m, int total_len, int flags, struct scm_cookie *scm);
    int (*mmap) (struct file *file, struct socket *sock, struct vm_area_struct * vma);
    + void (*fdclosed) (struct socket *sock, struct files_struct *files);
    };

    struct net_proto_family
    diff -ur -X /tmp/filexiytUc --minimal linux-2.4.0-test7-official/include/net/sock.h working-2.4.0-test7/include/net/sock.h
    --- linux-2.4.0-test7-official/include/net/sock.h Fri Aug 25 13:11:48 2000
    +++ working-2.4.0-test7/include/net/sock.h Fri Aug 25 13:39:12 2000
    @@ -708,6 +708,8 @@
    int *addr_len);
    int (*bind)(struct sock *sk,
    struct sockaddr *uaddr, int addr_len);
    + void (*fdclosed)(struct sock *sk,
    + struct files_struct *files);

    int (*backlog_rcv) (struct sock *sk,
    struct sk_buff *skb);
    @@ -836,6 +838,8 @@
    extern void sock_kfree_s(struct sock *sk, void *mem, int size);

    extern int copy_and_csum_toiovec(struct iovec *iov, struct sk_buff *skb, int hlen);
    +extern void sock_fdclosed(struct socket *sock, struct files_struct *files);
    +

    /*
    * Functions to fill in entries in struct proto_ops when a protocol
    diff -ur -X /tmp/filexiytUc --minimal linux-2.4.0-test7-official/net/core/sock.c working-2.4.0-test7/net/core/sock.c
    --- linux-2.4.0-test7-official/net/core/sock.c Fri Aug 25 13:11:48 2000
    +++ working-2.4.0-test7/net/core/sock.c Fri Aug 25 13:36:53 2000
    @@ -107,6 +107,7 @@
    #include <linux/interrupt.h>
    #include <linux/poll.h>
    #include <linux/init.h>
    +#include <linux/wait.h>

    #include <asm/uaccess.h>
    #include <asm/system.h>
    @@ -1062,6 +1063,28 @@
    {
    /* Mirror missing mmap method error code */
    return -ENODEV;
    +}
    +
    +void sock_fdclosed(struct socket *sock, struct files_struct *files)
    +{
    + struct sock *sk = sock->sk;
    + struct list_head *i;
    +
    + if (sk->sleep && waitqueue_active(sk->sleep)) {
    + unsigned long flags;
    +
    + /* Wake up everyone using same file table. */
    + wq_write_lock_irqsave(&sk->sleep->lock, flags);
    + for (i = sk->sleep->task_list.next;
    + i != &sk->sleep->task_list;
    + i = i->next) {
    + wait_queue_t *curr = list_entry(i, wait_queue_t,
    + task_list);
    + if (curr->task->files == files)
    + wake_up_process(curr->task);
    + }
    + wq_write_unlock_irqrestore(&sk->sleep->lock, flags);
    + }
    }

    /*
    diff -ur -X /tmp/filexiytUc --minimal linux-2.4.0-test7-official/net/ipv4/af_inet.c working-2.4.0-test7/net/ipv4/af_inet.c
    --- linux-2.4.0-test7-official/net/ipv4/af_inet.c Fri Aug 25 13:11:48 2000
    +++ working-2.4.0-test7/net/ipv4/af_inet.c Fri Aug 25 13:36:53 2000
    @@ -910,7 +910,8 @@
    getsockopt: inet_getsockopt,
    sendmsg: inet_sendmsg,
    recvmsg: inet_recvmsg,
    - mmap: sock_no_mmap
    + mmap: sock_no_mmap,
    + fdclosed: sock_fdclosed
    };

    struct proto_ops inet_dgram_ops = {
    @@ -931,6 +932,7 @@
    sendmsg: inet_sendmsg,
    recvmsg: inet_recvmsg,
    mmap: sock_no_mmap,
    + fdclosed: sock_fdclosed
    };

    struct net_proto_family inet_family_ops = {
    diff -ur -X /tmp/filexiytUc --minimal linux-2.4.0-test7-official/net/socket.c working-2.4.0-test7/net/socket.c
    --- linux-2.4.0-test7-official/net/socket.c Fri Aug 25 13:11:50 2000
    +++ working-2.4.0-test7/net/socket.c Fri Aug 25 13:36:53 2000
    @@ -104,7 +104,7 @@
    unsigned long count, loff_t *ppos);
    static ssize_t sock_writev(struct file *file, const struct iovec *vector,
    unsigned long count, loff_t *ppos);
    -
    +static void sock_do_fdclosed(struct file *file, struct files_struct *files);

    /*
    * Socket files have a set of 'special' operations as well as the generic file ones. These don't appear
    @@ -122,7 +122,8 @@
    release: sock_close,
    fasync: sock_fasync,
    readv: sock_readv,
    - writev: sock_writev
    + writev: sock_writev,
    + fdclosed: sock_do_fdclosed
    };

    /*
    @@ -706,6 +707,15 @@
    sock_fasync(-1, filp, 0);
    sock_release(socki_lookup(inode));
    return 0;
    +}
    +
    +static void sock_do_fdclosed(struct file *file, struct files_struct *files)
    +{
    + struct socket *sock;
    +
    + sock = socki_lookup(file->f_dentry->d_inode);
    + if (sock->ops->fdclosed)
    + sock->ops->fdclosed(sock, files);
    }

    /*
    ================================================================
    #include <sys/types.h>
    #include <sys/stat.h>
    #include <sys/socket.h>
    #include <sys/errno.h>
    #include <fcntl.h>
    #include <netinet/in.h>
    #include <netinet/udp.h>
    #include <netinet/ip.h>
    #include <stddef.h>
    #include <unistd.h>
    #include <stdio.h>
    #include <poll.h>
    #include <pthread.h>
    #include <sys/wait.h>
    /* We expect to be woken within 10 seconds. */
    void *woken(void *arg)
    {
    struct pollfd pfd;
    char buf[10];
    int r;
    time_t start = time(NULL);
    pfd.fd = (int)arg;
    pfd.events = POLLIN;

    /* 10 seconds. */
    r = poll(&pfd, 1, 10000);
    if (r == 1 && (pfd.revents & POLLNVAL)) {
    if (recv(pfd.fd, buf, 10, 0) >= 0)
    printf("FAILED: recv didn't fail\n");
    else if (errno != EBADF)
    printf("FAILED: errno %u != EBADF\n", errno);
    else if (time(NULL) >= start + 10)
    printf("FAILED: didn't wake me\n");
    else
    printf("woken: SUCCESS\n");
    } else {
    printf("FAILED: Poll didn't %s...\n",
    r == 0 ? "wake me" : "mark fd");
    }
    return NULL;
    }
    /* We expect NOT to be woken (dup'ed fd) */
    void *unwoken(void *arg)
    {
    struct pollfd pfd;
    pfd.fd = (int)arg;
    pfd.events = POLLIN;

    /* 10 seconds. */
    if (poll(&pfd, 1, 10000) != 0)
    printf("FAILED: Poll woke me!\n");
    else
    printf("unwoken: SUCCESS\n");
    return NULL;
    }
    /* We're a child: same fd, different file table, should not be woken */
    void unwoken_child(int fd)
    {
    struct pollfd pfd;
    pfd.fd = fd;
    pfd.events = POLLIN;

    /* 10 seconds. */
    if (poll(&pfd, 1, 10000) != 0)
    printf("FAILED: Poll woke child!\n");
    else
    printf("unwoken child: SUCCESS\n");
    exit(0);
    }
    int main(int argc, const char *argv[])
    {
    pthread_t waker, sleeper;
    int fd, fd2;
    if (argc != 2) {
    fprintf(stderr, "Usage: polltest [pipe|fifo|udp|tcp|raw|<filename>]\n");
    exit(1);
    }
    if (strcmp(argv[1], "udp") == 0) {
    struct sockaddr_in name;
    if ((fd = socket(AF_INET, SOCK_DGRAM, 0)) < 0) {
    perror("opening datagram socket");
    exit(1);
    }
    name.sin_family = AF_INET;
    name.sin_addr.s_addr = INADDR_ANY;
    name.sin_port = 0;
    if (bind(fd, &name, sizeof(name)) != 0) {
    perror("binding datagram socket");
    exit(1);
    }
    } else if (strcmp(argv[1], "tcp") == 0) {
    struct sockaddr_in name;
    if ((fd = socket(AF_INET, SOCK_STREAM, 0)) < 0) {
    perror("opening stream socket");
    exit(1);
    }
    name.sin_family = AF_INET;
    name.sin_addr.s_addr = INADDR_ANY;
    name.sin_port = 0;
    if (bind(fd, &name, sizeof(name)) != 0) {
    perror("binding stream socket");
    exit(1);
    }
    if (listen(fd, 5) < 0) {
    perror("listening on stream socket");
    exit(1);
    }
    } else if (strcmp(argv[1], "raw") == 0) {
    if ((fd = socket(AF_INET, SOCK_RAW, IPPROTO_RAW)) < 0) {
    perror("opening raw socket");
    exit(1);
    }
    } else if (strcmp(argv[1], "pipe") == 0) {
    int fds[2];
    if (pipe(fds) != 0) {
    perror("creating pipe");
    exit(1);
    }
    fd = fds[0];
    } else if (strcmp(argv[1], "fifo") == 0) {
    unlink("/tmp/fifotest");
    /* Linux doesn't block on mkfifo O_RDWR, so save a fork */
    if ((mkfifo("/tmp/fifotest", O_RDWR)) < 0) {
    perror("creating fifo");
    exit(1);
    }
    if ((fd = open("/tmp/fifotest", O_RDWR)) < 0) {
    perror("opening fifo");
    exit(1);
    }
    } else {
    /* Some character device, I presume */
    if ((fd = open(argv[1], O_RDONLY)) < 0) {
    perror("opening file");
    exit(1);
    }
    }
    if ((fd2 = dup(fd)) < 0) {
    perror("doing dup");
    exit(1);
    }
    switch (fork()) {
    case -1:
    perror("doing fork");
    exit(1);
    case 0:
    unwoken_child(fd);
    break;
    }
    if (pthread_create(&waker, NULL, woken, (void *)fd) != 0) {
    perror("Creating waker thread");
    exit(1);
    }
    if (pthread_create(&sleeper, NULL, unwoken, (void *)fd2) != 0) {
    perror("Creating sleeper thread");
    exit(1);
    }
    sleep(4);
    close(fd);
    pthread_join(waker, NULL);
    pthread_join(sleeper, NULL);
    wait(NULL);
    return 0;
    }
    ================
    #! /usr/bin/make
    # This needs to be a quiescent device (ie. won't wake up things on poll).
    CHARDEV:=/dev/ttyS1

    CFLAGS:=-Wall -W

    default: alltests

    alltests: polltest udptest tcptest rawtest pipetest fifotest chardevtest

    udptest:
    @echo UDP test; ./polltest udp

    tcptest:
    @echo TCP test; ./polltest tcp

    rawtest:
    @echo RAW test; ./polltest raw

    pipetest:
    @echo PIPE test; ./polltest pipe

    fifotest:
    @echo FIFO test; ./polltest fifo

    chardevtest:
    @echo CHARDEV test; ./polltest $(CHARDEV)

    polltest: polltest.c
    $(CC) $(CFLAGS) -o $@ $< -lpthread

    clean:
    @rm -f polltest
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-03-22 12:38    [W:0.046 / U:61.772 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site